feat: parallelize container monitor dependency requests#6748
Conversation
Co-authored-by: bgardiner <bgardiner@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Superseded by #6757, which uses bounded concurrency via the Two reasons for the re-implementation:
|
|
Closing — superseded by #6757 (bounded concurrency via SNYK_REQUEST_CONCURRENCY, simplified ordering). See comment above for rationale. |
Rewrite monitorDependencies in src/lib/ecosystems/monitor.ts to fan out per-ScanResult /monitor-dependencies PUTs in parallel via pMap, bounded by the SNYK_REQUEST_CONCURRENCY limit introduced in the previous commit (default 10). Per-ScanResult work is extracted into monitorOneScanResult for testability and clarity. Container images that produce many ScanResults (e.g. one per directory of JARs in fat-JAR-heavy images) previously incurred one full RTT per scan result, since the prior implementation used a nested for-loop with await. With bounded parallelism this collapses to ~ceil(N / concurrency) sequential batches, materially reducing wall-clock for large images. Error semantics are preserved: - 401 still throws AuthFailedError (terminates the run). - Other 4xx still throws MonitorError (terminates the run via pMap fail-fast). - 5xx and other non-4xx errors are accumulated per-ScanResult into the errors array, matching the prior continue-on-error behavior. Result order is preserved by pMap based on input order, so output remains deterministic regardless of completion order. Supersedes #6748, which used unbounded Promise.all (risking 500+ concurrent PUTs on large images) and a base-OS-first hybrid pattern that isn't needed once results are sorted by input order. New tests cover concurrency cap, env override, ordering, fail-fast, and non-4xx error accumulation.
Rewrite monitorDependencies in src/lib/ecosystems/monitor.ts to fan out per-ScanResult /monitor-dependencies PUTs in parallel via pMap, bounded by the SNYK_REQUEST_CONCURRENCY limit introduced in the previous commit (default 10). Per-ScanResult work is extracted into monitorOneScanResult for testability and clarity. Container images that produce many ScanResults (e.g. one per directory of JARs in fat-JAR-heavy images) previously incurred one full RTT per scan result, since the prior implementation used a nested for-loop with await. With bounded parallelism this collapses to ~ceil(N / concurrency) sequential batches, materially reducing wall-clock for large images. Error semantics are preserved: - 401 still throws AuthFailedError (terminates the run). - Other 4xx still throws MonitorError (terminates the run via pMap fail-fast). - 5xx and other non-4xx errors are accumulated per-ScanResult into the errors array, matching the prior continue-on-error behavior. Result order is preserved by pMap based on input order, so output remains deterministic regardless of completion order. Supersedes #6748, which used unbounded Promise.all (risking 500+ concurrent PUTs on large images) and a base-OS-first hybrid pattern that isn't needed once results are sorted by input order. New tests cover concurrency cap, env override, ordering, fail-fast, and non-4xx error accumulation.
Rewrite monitorDependencies in src/lib/ecosystems/monitor.ts to fan out per-ScanResult /monitor-dependencies PUTs in parallel via pMap, bounded by the SNYK_REQUEST_CONCURRENCY limit introduced in the previous commit (default 10). Per-ScanResult work is extracted into monitorOneScanResult for testability and clarity. Container images that produce many ScanResults (e.g. one per directory of JARs in fat-JAR-heavy images) previously incurred one full RTT per scan result, since the prior implementation used a nested for-loop with await. With bounded parallelism this collapses to ~ceil(N / concurrency) sequential batches, materially reducing wall-clock for large images. Error semantics are preserved: - 401 still throws AuthFailedError (terminates the run). - Other 4xx still throws MonitorError (terminates the run via pMap fail-fast). - 5xx and other non-4xx errors are accumulated per-ScanResult into the errors array, matching the prior continue-on-error behavior. Result order is preserved by pMap based on input order, so output remains deterministic regardless of completion order. Supersedes #6748, which used unbounded Promise.all (risking 500+ concurrent PUTs on large images) and a base-OS-first hybrid pattern that isn't needed once results are sorted by input order. New tests cover concurrency cap, env override, ordering, fail-fast, and non-4xx error accumulation.
Pull Request Submission Checklist
are release-note ready, emphasizing
what was changed, not how.
What does this PR do?
This PR improves performance for ecosystem/container monitoring by changing the
/monitor-dependenciesrequest flow from fully sequential to hybrid execution:ScanResultrequest first (base OS project ordering retained)ScanResultrequests in parallelAuthFailedErrorMonitorErrorWhere should the reviewer start?
src/lib/ecosystems/monitor.tstest/jest/unit/ecosystems-monitor-docker.spec.tsHow should this be manually tested?
npm run test:unit -- --runTestsByPath test/jest/unit/ecosystems-monitor-docker.spec.tssnyk container monitor <image>What's the product update that needs to be communicated to CLI users?
Container monitor execution is now faster for images that produce multiple scan results. The CLI now keeps base OS processing first and parallelizes the remaining dependency API requests.
Risk assessment (Low | Medium | High)?
Low. This is a scoped orchestration change with focused tests that verify ordering and existing error semantics.
Any background context you want to provide?
Large images that produce many scan results can experience long monitor durations due to sequential API calls; this change addresses that bottleneck.
What are the relevant tickets?
N/A