fix: Honor environment variables for memory threshold when WithConfiguration is omitted

[robertolopezlopez]

User description

Description

Provide description of this PR and changes.

Checklist

• Tests added and all succeed (make test)
• Regenerated mocks, etc. (make generate)
• Linted (make lint)
• Test your changes work for the CLI
  1. Clone / pull the latest CLI main.
  2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
     • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
  3. Run go mod tidy in the cliv2 directory.
  4. Run the CLI tests and do any required manual testing.
  5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
  • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

---

PR Type

Enhancement, Bug fix

---

Description

• Improve default configuration loading for Data.

• Prioritize environment variables for defaults.

• Enhance test coverage for configuration handling.

---

Diagram Walkthrough

flowchart LR
  Env["Environment Variables (e.g. INTERNAL_IN_MEMORY_THRESHOLD_BYTES)"]
  Defaults["Default Configuration Values (e.g. 512MB)"]
  ExplicitConfig["Explicit Configuration (WithConfiguration)"]
  DataCreation["NewData() call"]
  DataImpl["DataImpl initialization"]

  Env -- "Used for defaults" --> DataCreation
  Defaults -- "Used for defaults" --> DataCreation
  ExplicitConfig -- "Overrides defaults" --> DataCreation
  DataCreation -- "Initializes" --> DataImpl

Loading

File Walkthrough

	Relevant files
Enhancement	dataimpl.go Apply env-backed configuration defaults for Data initialization pkg/workflow/dataimpl.go Modified NewData to initialize DataImpl with configuration that automatically loads environment variables. Introduced default values for IN_MEMORY_THRESHOLD_BYTES using constants.SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB when environment variables are not explicitly set. Ensured that explicit WithConfiguration options provided by the caller will override environment-backed defaults. +5/-5
Tests	dataimpl_test.go Expand tests for Data configuration and environment variable handling pkg/workflow/dataimpl_test.go Added helper functions payloadLocationType and inMemoryThresholdBytes for easier test assertions. Introduced new test cases to verify that environment variables are honored when configuration is omitted. Added tests for default threshold values (512MB) and ensuring caller-provided configuration overrides environment settings. Included tests for NewDataFromInput to ensure it also inherits env-backed defaults. +78/-35

---

MANUAL TESTING

Local build.

⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ export SNYK_TMP_PATH=/tmp/snyk-cli-1509-test
                                 export INTERNAL_IN_MEMORY_THRESHOLD_BYTES=1
                                 export INTERNAL_CLEANUP_GLOBAL_TEMP_DIR_ENABLED=false
                                 export SNYK_LOG_LEVEL=trace
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ echo $SNYK_TMP_PATH
/tmp/snyk-cli-1509-test
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ echo $SNYK_LOG_LEVEL
trace
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ export SNYK_TOKEN=(../binary-releases/snyk-macos-arm64 config get INTERNAL_OAUTH_TOKEN_STORAGE)
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ echo $SNYK_TOKEN 
...REDACTED
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ echo $SNYK_API
https://api.dev.snyk.io/
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ rm -rf "$SNYK_TMP_PATH" && mkdir -p "$SNYK_TMP_PATH"
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ ../binary-releases/snyk-macos-arm64 config get test --experimental --json
⋊> ~/g/c/cliv2 on fix/CLI-1509 ⨯ ls -la "$SNYK_TMP_PATH" 
                                 find "$SNYK_TMP_PATH" -name 'workflow.*'
total 8
drwxr-xr-x@  4 roberto  wheel  128 Jun 11 16:19 ./
drwxrwxrwt  22 root     wheel  704 Jun 11 16:18 ../
drwxr-xr-x@  3 roberto  wheel   96 Jun 11 16:19 1.1306.0-preview.08381bd4215281401cd4e12b20ee53f3926f562d.oss/
-rwxr-xr-x@  1 roberto  wheel  733 Jun 11 16:19 workflow.analytics.report.1992018518*
/tmp/snyk-cli-1509-test/workflow.analytics.report.1992018518

---

RELATED

There was a previous PR trying to approach this issue which includes a quite similar approach: there WithConfiguration() checks if IN_MEMORY_THRESHOLD_BYTES has been set - if not, it assigns a value.

This PR just sets the same default value as during the app bootstrap (512 MB).

[snyk-io]

✅ 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-io]

✅ 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.

[robertolopezlopez]

/describe

[snyk-pr-review-bot]

PR Description updated to latest commit (f6732d6)

[robertolopezlopez]

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Unit Mismatch 🟠 [major]
The default memory threshold is initialized using constants.SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB, but the inMemoryThreshold field is documented as being in 'bytes' (see line 21) and is used directly in byte-count comparisons (line 290). Given the constant name and the comment in pkg/app/app.go (line 347), the value is likely 512. This results in a default threshold of 512 bytes rather than 512 megabytes, causing nearly all byte-slice payloads to be unexpectedly written to disk.

c.AddDefaultValue(configuration.IN_MEMORY_THRESHOLD_BYTES,
	configuration.StandardDefaultValueFunction(constants.SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB))

Resource Leak 🟠 [major]
By removing the previous default threshold of -1 (which explicitly disabled disk spilling), this PR enables the feature by default for all Data objects. As shown in pkg/workflow/types.go, the Data interface lacks a Close() or Cleanup() method, and writeDataToDisk (line 296) does not implement any automatic deletion. This will cause applications to leak temporary files and potentially exhaust disk space, especially given the likely unit mismatch that triggers spills for very small payloads.

c := configuration.NewWithOpts(configuration.WithAutomaticEnv())
c.AddDefaultValue(configuration.IN_MEMORY_THRESHOLD_BYTES,
	configuration.StandardDefaultValueFunction(constants.SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB))
opts = append([]Option{WithConfiguration(c)}, opts...)

📚 Repository Context Analyzed

This review considered 7 relevant code sections from 5 files (average relevance: 0.93)

• pkg/mocks/workflow.go (lines 1-238)
• pkg/app/app.go (2 segments, lines 1-197)
• pkg/configuration/configuration.go (lines 1-269)
• pkg/workflow/types.go (lines 1-115)
• pkg/workflow/dataimpl.go (2 segments, lines 1-259)

Unit Mismatch

False positive: SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB is 512 * 1024 * 1024 bytes.

Resource leak

Half false.

• Removing -1 enables the default 512 MB as app.initConfiguration. 512 MB is large enough memory to affirm that just very large byte payloads may spill.
• The CLI removes the managed temp directory on exit, so after that there problem solved.
  • In any case, it is always possible to get the ols "in memory" behavior by passing WithConfiguration with -1 (see dataimpl_test.go:243 "caller WithConfiguration overrides env-backed default")

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Unit Mismatch 🟠 [major] The configuration key configuration.IN_MEMORY_THRESHOLD_BYTES expects a value in bytes (as documented in the DataImpl struct), but it is being assigned constants.SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB. The constant name and related comments in pkg/app/app.go suggest a value in Megabytes (e.g., 512). Without a conversion to bytes (multiplication by 1024*1024), the resulting threshold will be extremely low (512 bytes), causing virtually all payloads to be written to disk. This will cause significant performance degradation due to unnecessary disk I/O for small payloads. c.AddDefaultValue(configuration.IN_MEMORY_THRESHOLD_BYTES, configuration.StandardDefaultValueFunction(constants.SNYK_DEFAULT_IN_MEMORY_THRESHOLD_MB)) Performance Overhead 🟡 [minor] newDataWith now creates a new Configuration instance (which involves initializing a viper.Viper object) every time it is called. Since NewData and NewDataFromInput are fundamental constructors used frequently throughout the lifecycle of a workflow, this adds non-trivial CPU and memory overhead per data object. Consider using a shared default configuration or directly checking environment variables if WithConfiguration is omitted. c := configuration.NewWithOpts(configuration.WithAutomaticEnv())

📚 Repository Context Analyzed This review considered 7 relevant code sections from 5 files (average relevance: 0.93) pkg/app/app.go (2 segments, lines 1-197) pkg/mocks/workflow.go (lines 1-238) pkg/configuration/configuration.go (lines 1-269) pkg/local_workflows/local_models/models.gen.go (lines 1-239) pkg/workflow/dataimpl.go (2 segments, lines 1-259)