fix: allow the context on env load to be canceled [IDE-1791] by rrama · Pull Request #564 · snyk/go-application-framework

rrama · 2026-03-05T10:20:27Z

Plus other nice to have options can be passed.
Needed for snyk/snyk-ls#1173

Plus other nice to have options can be passed.

snyk-io · 2026-03-05T10:20:39Z

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

Status	Scanner	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 · 2026-03-05T10:20:42Z

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

Status	Scanner	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.

bastiandoetsch · 2026-03-09T12:31:07Z

pkg/envvars/environment.go

+	logger            *zerolog.Logger
+}
+
+type LoadConfiguredEnvironmentOptions func(opts *loadConfiguredEnvironmentOptions)


Hm, don't we need to export the loadConfiguredEnvironmentOptions struct? I wonder how external processes should work with the pointer to the struct?

Ah, actually we don't need either exported, we only need the pre-defined WithBlah functions exported, as that is all the external callers should interact with.

Although in theory someone may want to create a slice of the WithBlah functions, but if someone does want to do something that peculiar, they can come it and change it to be exported.

Made it unexported.

bastiandoetsch · 2026-03-09T12:31:23Z

/review

bastiandoetsch · 2026-03-09T12:32:26Z

pkg/envvars/environment.go

+	}
+}
+
+func WithCustomConfigFiles(customConfigFiles []string, workingDirectory string) LoadConfiguredEnvironmentOptions {


For ease of use:

shouldn't the custom config files be just absolute paths?

Is that not just more effort for the caller? They would need to join all the .snyk.env, .envrc, etc. with the working dir path before calling.
Hmm, as a best of both, I could rename this one to WithRelativeCustomConfigFiles and then do a second function that is just for absolute config files.

Made it so you have to pass absolute paths, but I added a util function that converts relative paths to absolute paths to make life easier for the callers.

bastiandoetsch · 2026-03-09T12:33:39Z

pkg/envvars/environment.go

+// The Bash env PATH is appended to the existing PATH (as a fallback), any other new PATH read is prepended (preferential).
+func LoadConfiguredEnvironmentWithOptions(opts ...LoadConfiguredEnvironmentOptions) {
+	options := loadConfiguredEnvironmentOptions{
+		ctx:    context.Background(),


I guess, this should be a context with timeout of e.g. 5s

getEnvFromShell already puts a 5 second timeout on the context. But we could shift the timeout up to encompass the entirety of the loading env if that's better?

I tried moving the timeout up, but then you have to decide if you want it before or after getting the mutex, so I decided in the end to leave it as it currently is with only the timeout in getEnvFromShell. A caller can always specify a timeout on the context they pass.

Now requires absolute paths. But I also added a util function to make paths absolute. Added an overall timeout for the entire `LoadConfiguredEnvironmentWithOptions` function.

It was clashing with the mutex locking. The caller can just provide a timeout on their context. Make logging more accurate.

snyk-pr-review-bot · 2026-03-10T14:42:42Z

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Inefficient Lock Acquisition 🟠 [major] The function `LoadConfiguredEnvironmentWithOptions` acquires a package-level mutex `mu` before checking if the provided context `options.ctx` is already canceled. Since `sync.Mutex.Lock()` is not context-aware, a caller passing a canceled context will still block until any ongoing environment loading (which can take up to 10 seconds due to shell execution timeouts) finishes and releases the lock. This defeats the primary purpose of the PR, which is to allow the environment loading process to be responsive to cancellation. mu.Lock() defer func() { mu.Unlock() Redundant Logger Wrapping 🟡 [minor] In `LoadConfiguredEnvironmentWithOptions`, a new logger is created with a "method" field. However, `getEnvFromShell` is then called with `options.logger` (the original logger) instead of the decorated `logger`. This results in `getEnvFromShell`'s logs missing the "method" context established at the start of the calling function, leading to inconsistent log metadata across the execution flow. bashOutput := getEnvFromShell(options.ctx, options.logger, "bash")
📚 Repository Context Analyzed This review considered 15 relevant code sections from 15 files (average relevance: 1.03) pkg/utils/paths.go (lines 1-17) pkg/local_workflows/connectivity_check_extension/connectivity/types.go (lines 1-188) pkg/envvars/environment.go (lines 1-239) pkg/mocks/userinterface.go (lines 1-113) pkg/configuration/storage.go (lines 1-159) pkg/analytics/analytics.go (lines 1-374) pkg/ui/userinterface.go (lines 1-33) ... and 8 more file(s)

rrama · 2026-03-12T14:44:50Z

The LS PR is blocked, I do want to mark the other function as deprecated, so I will wait to merge this until then.

fix: allow the context on env load to be canceled

96ae016

Plus other nice to have options can be passed.

rrama changed the title ~~fix: allow the context on env load to be canceled~~ fix: allow the context on env load to be canceled [IDE-1791] Mar 5, 2026

rrama marked this pull request as ready for review March 5, 2026 11:11

rrama requested review from a team as code owners March 5, 2026 11:11

rrama mentioned this pull request Mar 5, 2026

fix: server lifecycle context [IDE-1791] snyk/snyk-ls#1173

Open

5 tasks

bastiandoetsch reviewed Mar 9, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

bastiandoetsch reviewed Mar 9, 2026

View reviewed changes

bastiandoetsch approved these changes Mar 9, 2026

View reviewed changes

reafctor: change custom config file passing

9dc3862

Now requires absolute paths. But I also added a util function to make paths absolute. Added an overall timeout for the entire `LoadConfiguredEnvironmentWithOptions` function.

This comment has been minimized.

Sign in to view

test: make path util tests work on Windows

1558299

This comment has been minimized.

Sign in to view

fix: remove overall timeout

dbfe2da

It was clashing with the mutex locking. The caller can just provide a timeout on their context. Make logging more accurate.

rrama added the 🚧 Blocked PR is blocked waiting for something else first label Mar 12, 2026

Conversation

rrama commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snyk-io bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

snyk-io bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bastiandoetsch commented Mar 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrama Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

snyk-pr-review-bot bot commented Mar 10, 2026

PR Reviewer Guide 🔍

Uh oh!

rrama commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rrama commented Mar 5, 2026 •

edited

Loading

snyk-io bot commented Mar 5, 2026 •

edited

Loading

snyk-io bot commented Mar 5, 2026 •

edited

Loading

rrama Mar 10, 2026 •

edited

Loading