Skip to content

Perf/bundle parallelization - #164

Draft
apzuk3 wants to merge 4 commits into
mainfrom
perf/bundle-parallelization
Draft

Perf/bundle parallelization#164
apzuk3 wants to merge 4 commits into
mainfrom
perf/bundle-parallelization

Conversation

@apzuk3

@apzuk3 apzuk3 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Description

Provide description of this PR and changes, if linked Jira ticket doesn't cover it in full.

Checklist

  • Tests added and all succeed
  • Linted
  • README.md updated, if user-facing

🚨After having merged, please update the snyk-ls and CLI go.mod to pull in latest client.

z4ce and others added 3 commits July 16, 2026 16:20
Replace the two-pass file discovery (GetAllFiles followed by
GetFilteredFiles) with GetFilteredFilesSingleWalk, which traverses the
project tree once and prunes ignored directories (e.g. node_modules,
.git) instead of walking the whole tree and glob-matching every file
underneath them. That traversal was the dominant cost on real projects.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Process files with a pool of workers when creating a bundle. The
per-file work (stat, read, hash) is I/O-bound and independent across
files, so oversubscribing the CPUs keeps the disk busy while syscalls
block — this matters most on Windows, where per-file syscalls are
comparatively expensive. Results are merged under a mutex, and the
lazy filter load is guarded so concurrent workers fetch filters exactly
once. Each file is opened a single time and stat'd via the open handle
to avoid a redundant filesystem lookup.

Also deduplicate the UTF-8 conversion in BundleFileFrom: the content is
now converted once and reused for both the hash and the bundle content,
via the new util.HashContent helper, instead of converting twice.

Add a benchmark covering bundle creation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The committed replace directive pointed at a nonexistent local path
(/Users/ianzink/git/gaf-v0.3.2-local), which broke `go mod tidy`, build,
unit tests, and the Snyk SCA/license checks in CI. The parallelization
work also depends on `GetFilteredFilesSingleWalk`, which currently only
exists on the companion GAF branch (snyk/go-application-framework#657).

Repoint the replace directive at that GAF commit
(github.com/z4ce/go-application-framework@5ac64af, pseudo-version
v0.0.0-20260716054810-5ac64afcd596) and update go.sum so the module
graph resolves and CI can build/test the change.

NOTE: this is a temporary measure to unblock CI while both PRs are
drafts. Before merge, GAF #657 should land and this should be repinned
to a real snyk/go-application-framework version with the replace removed.
@apzuk3
apzuk3 requested a review from a team as a code owner July 21, 2026 10:51
@snyk-io

snyk-io Bot commented Jul 21, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues
Secrets 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@apzuk3
apzuk3 marked this pull request as draft July 21, 2026 10:51
@snyk-io

snyk-io Bot commented Jul 21, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 Security concerns

Yes. The use of a personal fork (z4ce/go-application-framework) in go.mod as a replacement for a core Snyk framework is a high-risk security concern. It allows unverified code to be executed during the file scanning process, potentially leading to exfiltration or supply chain compromise if the fork is malicious or later compromised.

⚡ Recommended focus areas for review

Systemic Risk 🔴 [critical]

The replace directive points to a personal fork (github.com/z4ce/go-application-framework) rather than an official Snyk repository. This introduces a significant security and maintenance risk, as this fork is not subject to the same access controls and audit requirements as the main Snyk organization. This dependency is also used for the critical file filtering logic in native_workflow.go.

replace github.com/snyk/go-application-framework => github.com/z4ce/go-application-framework v0.0.0-20260716054810-5ac64afcd596
Race Condition 🟠 [major]

In the create method, the loop iterates createWorkerCount() times to spawn workers, but the setErr function (which calls cancel()) is invoked from goroutines. If an error occurs in one worker, it cancels the processCtx, but the main create function continues to spawn remaining workers in the loop if they haven't started yet. While not a crash, it wastes resources spawning workers that will immediately exit due to processCtx.Err() != nil.

	return
}
Lost Error Context 🟡 [minor]

The refactored BundleFileFrom now swallows the error from util.ConvertToUTF8(content). It falls back to the original content but no longer returns the err to the caller, whereas the previous implementation explicitly returned the conversion error in the fileErr variable. This masks encoding issues from upstream logging or error reporting.

utf8Content, err := util.ConvertToUTF8(content)
if err != nil {
	utf8Content = content
}
📚 Repository Context Analyzed

This review considered 22 relevant code sections from 13 files (average relevance: 0.96)

🤖 Repository instructions applied (from AGENTS.md)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants