Skip to content

fix(analyzer): bound file analysis workers - #8058

Merged
cx-artur-ribeiro merged 3 commits into
Checkmarx:masterfrom
omribz156:codex/analyzer-bounded-workers
Sep 7, 2026
Merged

cx-artur-ribeiro merged 3 commits into
Checkmarx:masterfrom
omribz156:codex/analyzer-bounded-workers

Conversation

@omribz156

Copy link
Copy Markdown
Contributor

Closes #8046

Reason for Proposed Changes

  • The analyzer currently starts one goroutine per candidate file.
  • On very large repositories, that can create tens of thousands of goroutines during "Preparing Scan Assets" and trigger the thread-exhaustion crash reported in the issue.

Proposed Changes

  • Replace per-file goroutine spawning with a bounded worker pool.
  • Size the pool from GOMAXPROCS, capped at 128 workers and never larger than the candidate file count.
  • Add unit coverage for the worker-count helper.

Verification

  • go test ./pkg/analyzer -count=1
  • go test ./pkg/scan -run "Test.*Analyze|Test.*Init|Test.*Prepare" -count=1
  • git diff --check

This was implemented with Codex assistance, with the final patch kept focused and manually reviewed.

I submit this contribution under the Apache-2.0 license.

@devAL3X

devAL3X commented May 27, 2026

Copy link
Copy Markdown

I have tested this fix against my repo where the issue was occurring - the issue is no longer reproducible

@cx-artur-ribeiro

cx-artur-ribeiro commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

Hi @omribz156,
Thanks for the contribution 🙏 !

I've also verified this resolves the thread-exhaustion crash cleanly and the bounded-pool wiring (channel-fed workers, wg.Add/wg.Done moved to the pool level) is correct, including preserving the existing unwanted-channel drain race handling in computeValues that I've introduced earlier this year.

One addition I would propose: the added test to analyzer_test only covers analyzerWorkerCount as a pure function. Nothing exercises Analyze() itself under a file count that forces multiple files through a single pooled worker, so a regression that reintroduced go a.worker(...) per file would pass this test suite untouched.
I suggest adding the test bellow alongside it, in pkg/analyzer_test.go.

Let me know what you think and thanks again for your help improving kics!
Thank you as well @devAL3X for exposing the problem and taking the time to explain what the problem was as well.

P.S: I can address the changes on my side and close this PR once we merge the changes, mentioning both the issue and the original pull request.

Comment thread pkg/analyzer/analyzer_test.go
@cx-artur-ribeiro

Copy link
Copy Markdown
Contributor

Commits do need to be signed in order for any pull request to be merged.

If this is something that you cannot achieve on your side, I will open the PR on my side and tag you, as well as the pull request and related issue as well.

Let me know if you could sign the commits on your side @omribz156.

@cx-rui-araujo

Copy link
Copy Markdown
Contributor

Hi @omribz156,
Thanks for the contribution 🙏 !

The bounded pool implementation looks good.
I have one suggestion: could we make the 128 worker cap configurable via a MaxAnalyzerWorkers field in the Analyzer struct? This would let users who consume the package adjust it for their specific environments.
I added comments with the suggestion in case you wanna commit via GitHub.
P.S: I can address these changes on my side in a different PR after merging this one (so you can resolve my comments if you feel so).

Let me know what you think and thanks again for your help improving KICS!
Also, as Artur said, thank you as well @devAL3X for exposing the problem and taking the time to explain what the problem was as well.

Comment thread pkg/analyzer/analyzer.go
Comment thread pkg/analyzer/analyzer.go Outdated
Comment thread pkg/analyzer/analyzer.go Outdated
@cx-artur-ribeiro
cx-artur-ribeiro force-pushed the codex/analyzer-bounded-workers branch from 2880735 to 8724dcd Compare September 7, 2026 14:07

@cx-rui-araujo cx-rui-araujo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐐

@cx-artur-ribeiro
cx-artur-ribeiro merged commit f118059 into Checkmarx:master Sep 7, 2026
5 of 35 checks passed
cx-andre-pereira pushed a commit that referenced this pull request Sep 8, 2026
* fix(analyzer): bound file analysis workers

* test: cover bounded analyzer worker concurrency

* update: add MaxAnalyzerWorkers to Analyzer providing better usage for library embedders

---------

Co-authored-by: Artur Ribeiro <153724638+cx-artur-ribeiro@users.noreply.github.com>
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
kics 2.2.0

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>## What's Changed
* docs(release): update queries catalog, index and dockerfile for upcoming release by @cx-artur-ribeiro in Checkmarx/kics#8094
* fix(query): added missing case to "Last User is Root" Dockerfile query. by @cx-andre-pereira in Checkmarx/kics#8095
* feat(queries): new queries to check if synapse workspace managed virtual network is enabled by @cx-ricardo-jesus in Checkmarx/kics#8097
* refactor(descriptions): remove pkg/descriptions and related CLI flag by @cx-ricardo-jesus in Checkmarx/kics#8092
* CISO-1264 - Update GitHub Actions runner labels by @cx-jonathan-hartman in Checkmarx/kics#8093
* fix(query): Fix for Missing Backslash Support on Copy_With_More_Than_Two_Arguments_Not_Ending_With_Slash query by @cx-andre-pereira in Checkmarx/kics#8099
* fix(actions): refactor gh actions to fix CI by @cx-artur-ribeiro in Checkmarx/kics#8098
* fix(query): change to keyExpectedValue on ARM 'default_azure_storage_account_network_access_is_too_permissive' query by @cx-andre-pereira in Checkmarx/kics#8101
* update(version): rename VERSION build-arg to ENGINE_VERSION by @cx-artur-ribeiro in Checkmarx/kics#8102
* fix(query): correct keyExpectedValue and keyActualValue in redshift_not_encrypted by @cx-ricardo-jesus in Checkmarx/kics#8105
* fix(analyzer): bound file analysis workers by @omribz156 in Checkmarx/kics#8058
* fix(actions): remove outdated actions and update documentation accordingly by @cx-artur-ribeiro in Checkmarx/kics#8110
* fix(action): remove concurrent group from run projects github action by @cx-artur-ribeiro in Checkmarx/kics#8111
* fix(test): normalize timestamp comparison in TestInitCycloneDxReport by @cx-artur-ribeiro in Checkmarx/kics#8113
* fix(actions): fix security vulnerabilities and update ci with new enforced rules by @cx-artur-ribeiro in Checkmarx/kics#8118
* fix(release): gate kics release workflows behind release environment by @cx-lior-poterman in Checkmarx/kics#8108
* fix(validator): update queries validator for cwe and risk score fields by @cx-artur-ribeiro in Checkmarx/kics#8028
* fix(version): new available version with additional v prefix by @cx-artur-ribeiro in Checkmarx/kics#8119
* fix(analyzer): improvement to dockerfile scanning by @cx-andre-pereira in Checkmarx/kics#8114
* chore(release): removed unused goreleaser configuration files by @cx-ricardo-jesus in Checkmarx/kics#8122
* fix(filesystem): skip cache files by extension by @cx-laura-rodrigues in Checkmarx/kics#8112
* fix(query): changed all dockerfile queries for case insensitive support of dockerfile commands by @cx-andre-pereira in Checkmarx/kics#8115
* docs(release): update queries catalog, index and dockerfile for 2.2.0 by @cx-artur-ribeiro in Checkmarx/kics#8126

## New Contributors
* @cx-jonathan-hartman made their first contribution in Checkmarx/kics#8093
* @omribz156 made their first contribution in Checkmarx/kics#8058
* @cx-lior-poterman made their first contribution in Checkmarx/kics#8108

**Full Changelog**: https://github.com/Checkmarx/kics/compare/v2.1.21...v2.2.0</pre>
  <p>View the full release notes at <a href="https://github.com/Checkmarx/kics/releases/tag/v2.2.0">https://github.com/Checkmarx/kics/releases/tag/v2.2.0</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!20473
cx-andre-pereira pushed a commit that referenced this pull request Sep 29, 2026
* fix(analyzer): bound file analysis workers

* test: cover bounded analyzer worker concurrency

* update: add MaxAnalyzerWorkers to Analyzer providing better usage for library embedders

---------

Co-authored-by: Artur Ribeiro <153724638+cx-artur-ribeiro@users.noreply.github.com>
cx-andre-pereira pushed a commit that referenced this pull request Sep 29, 2026
* fix(analyzer): bound file analysis workers

* test: cover bounded analyzer worker concurrency

* update: add MaxAnalyzerWorkers to Analyzer providing better usage for library embedders

---------

Co-authored-by: Artur Ribeiro <153724638+cx-artur-ribeiro@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): thread exhaustion when scanning large repositories

4 participants