refactor(di): remove process-global singletons to enable t.Parallel() [IDE-2036]#1319
refactor(di): remove process-global singletons to enable t.Parallel() [IDE-2036]#1319bastiandoetsch wants to merge 42 commits into
Conversation
✅ 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. |
This comment has been minimized.
This comment has been minimized.
e936705 to
186a478
Compare
|
/describe |
6286cdb to
c9fa4cd
Compare
|
PR Description updated to latest commit (c9fa4cd) |
This comment has been minimized.
This comment has been minimized.
|
/describe |
|
PR Description updated to latest commit (c9fa4cd) |
|
/describe |
|
PR Description updated to latest commit (4aadc51) |
This comment has been minimized.
This comment has been minimized.
|
/describe |
|
/describe |
|
PR Description updated to latest commit (d2db8ed) |
…el + complete scanCtx threading [IDE-2036] Replaces the process-global progress.ToServerProgressChannel and global tracker registry with a per-server progress.Tracker (owner: channel + token->Task registry, Cancel/IsCanceled) and progress.Task (per-operation handle, implements ui.ProgressBar); token cancellation resolves per-server via the context-injected Tracker. Migrates the downloader and the GAF extension entrypoint off the global ctor and all scanner/test callsites onto per-server (drained) Trackers. Threads the server-lifetime scanCtx into HandleFolders (initialized) and the workspace/folder/clear-cache scan commands so every background scan respects shutdown cancellation. Also: thread scanCtx into the didSave + didChangeWorkspaceFolders handlers; share one process-global CLI concurrency semaphore across executors; replace the order-dependent sync.Once SNYK_API env with a per-server WithAPIEndpoint config option.
|
/describe |
|
PR Description updated to latest commit (d2db8ed) |
|
/describe |
|
/describe |
1 similar comment
|
/describe |
|
PR Description updated to latest commit (3e13561) |
2 similar comments
|
PR Description updated to latest commit (3e13561) |
|
PR Description updated to latest commit (3e13561) |
This comment has been minimized.
This comment has been minimized.
# Conflicts: # application/server/configuration_test.go
|
/describe |
|
PR Description updated to latest commit (0df9634) |
This comment has been minimized.
This comment has been minimized.
…i 2.12.2 [IDE-2036] - scan_context_test.go: build the didSave intercept path via filepath.Join and store interceptPath as uri.PathFromUri(uri.PathToUri(fakePath)) so it matches the handler's computed path on Windows (was a forward-slash literal + exact string match, failing integration/smoke on windows-latest). - internal/util/values.go: reflect.Ptr -> reflect.Pointer (deprecated since Go 1.18). - .golangci.yaml: targeted exclusion for the govet 'inline' analyzer false-positive on generic stdlib calls (slices.Contains/ContainsFunc) — "type parameter inference is not yet supported", no fix available; real inline findings stay enforced. - Makefile: bump pinned golangci-lint v2.10.1 -> v2.12.2 so CI's make lint matches the analyzer set; make lint reports 0 issues whole-repo.
|
/describe |
|
PR Description updated to latest commit (44966cf) |
PR Reviewer Guide 🔍
|
|
/describe |
|
PR Description updated to latest commit (44966cf) |
rrama
left a comment
There was a problem hiding this comment.
I haven't finished reviewing, but I am going to leave the comments I have so far. I haven't prioritised them or anything, but I'm sure you can use AI to figure out what you want to do or not.
Also, given the time I am leaving some of the AI comments without first checking them myself and some without me even reading them 🙈
| // Access them via di.Installer() / di.Initializer() until those global | ||
| // accessors are retired. | ||
| // Note: Initializer is intentionally absent — it is a process-lifecycle | ||
| // dependency used during startup only. |
There was a problem hiding this comment.
Will it not be added in the future?
| // token→task registry [IDE-2036]. Each server instance has its own Tracker so | ||
| // progress events from one server cannot leak to another server's listener. | ||
| ProgressTracker *progress.Tracker | ||
| // ScanCtx is a server-lifetime context for workspace scan goroutines. |
There was a problem hiding this comment.
I was planning to do this as part of #1173 after we merge this, can we close that PR and Jira ticket?
| // are safe to run concurrently without a data race. | ||
| func RealDependencies(engine workflow.Engine, tokenService types.TokenService) Dependencies { | ||
| owner := progress.NewTracker(engine.GetLogger()) | ||
| deps, _, _ := buildDependencies(engine, tokenService, owner) |
There was a problem hiding this comment.
Should this func move to a test util package and call t.Cleanup(treeEmitter.Dispose)? It does only seem to be one channel left open per tree emitter, so not the worst test-to-test leak.
Or return the treeEmitter and let the caller do it.
| } | ||
|
|
||
| // Resolve the per-test progress tracker. | ||
| // Priority: overrideDeps.ProgressTracker > default fresh tracker. |
There was a problem hiding this comment.
Nitpick: AI comment adding no value.
| } else { | ||
| featureFlagService = featureflag.New(gafConfiguration, logger, engine, configResolver) | ||
| // Default: create a fresh per-test tracker. The process-global | ||
| // progress channel has been removed [IDE-2036]. |
There was a problem hiding this comment.
Nitpick: Not needed to be mentioned in the code.
| t.Setenv("SNYK_LOG_LEVEL", "debug") | ||
| prevLogLevel := os.Getenv("SNYK_LOG_LEVEL") | ||
| os.Setenv("SNYK_LOG_LEVEL", "debug") //nolint:usetesting // t.Setenv panics after t.Parallel() | ||
| t.Cleanup(func() { _ = os.Setenv("SNYK_LOG_LEVEL", prevLogLevel) }) //nolint:usetesting // t.Setenv panics after t.Parallel() |
There was a problem hiding this comment.
Why not just not make it t.Parallel() (since it isn't if you are messing with env vars) or stop using the env var...
| Msg("Code scanner: files to be scanned") | ||
|
|
||
| t := progress.NewTracker(true, sc.engine.GetLogger()) | ||
| t := progress.NewTaskWithChannel(sc.progressChannel, true, sc.engine.GetLogger()) |
There was a problem hiding this comment.
AI comment (unread by me):
Scanner progress Tasks created via NewTaskWithChannel are not registered with the per-server Tracker, breaking client-initiated cancel
All three product scanners (Code, OSS, IaC) and the code-client-go tracker factory create progress Tasks via
NewTaskWithChannel()which setsowner: niland does not register the task in anyTracker.tasksregistry. When the IDE user clicks "Cancel" on a progress bar,windowWorkDoneProgressCancelHandler(application/server/server.go:1249) callsowner.Cancel(params.Token)which looks up the token inowner.tasks. Since scanner Tasks were never registered, the lookup fails silently and the cancel signal is never delivered to the task'scancelChannel.Before this PR,
progress.NewTracker(true, logger)registered every task in the globaltrackersmap, and the oldprogress.Cancel(token)could find and cancel it. After this PR the global map was removed, but the replacement per-server registry is only populated byTracker.New()— never byNewTaskWithChannel(). The same issue affects:
infrastructure/iac/iac.go:159infrastructure/oss/cli_scanner.go:253infrastructure/code/code_tracker.go:41(code-client-go upload progress)
| // Tracker. Use it when you already hold a channel reference (e.g. from | ||
| // Tracker.Channel()) and need a Task with a specific cancellable setting but | ||
| // no owner-managed registry entry. | ||
| func NewTaskWithChannel(channel chan types.ProgressParams, cancellable bool, logger *zerolog.Logger) *Task { |
There was a problem hiding this comment.
AI says:
NewTaskWithChannel creates ownerless tasks where IsCanceled() is always false
Tasks created via
NewTaskWithChannel(internal/progress/progress.go:53) haveowner == nil, soTask.IsCanceled()(task.go:72-77) always returnsfalse. This means cancellation via the owner registry is not possible for these tasks. However, every callsite that previously relied ont.IsCanceled()has been changed:code.go:289-290now checksctx.Err() != nilinstead, and the scanners useCancelOrDonewhich listens on the cancel channel directly. So the behavioral change is intentional and correct — but it is a subtle semantic difference from the old code where all tasks were globally registered. Future callers ofNewTaskWithChannelshould be aware thatIsCanceled()is a no-op for these tasks.
Probably worth updating this func's Go doc to call this out.
|
|
||
| // decouple scheduled scan from session but keep context values | ||
| newCtx := ctx2.Clone(ctx, context.Background()) | ||
| newCtx := context.WithoutCancel(ctx) |
There was a problem hiding this comment.
AI comment:
OSS scheduleRefreshScan:
newCtx.Err()check is always nilAt
infrastructure/oss/cli_scanner.go:614, the code checksnewCtx.Err() != nilwherenewCtxis created viacontext.WithoutCancel(ctx)at line 603. SinceWithoutCancelproduces a context that is never canceled, this check will always return nil. This is a pre-existing logic issue — the oldctx2.Clone(ctx, context.Background())had the exact same behavior (background context is never canceled). The actual cancellation logic correctly uses the originalctx.Done()channel on line 627. The deadErr()check is harmless but misleading.
Not your code smell and doesn't really matter, so up to you if you do anything about it.
| engine.SetUserInterface(user_interface.NewLsUserInterface( | ||
| user_interface.WithLogger(engine.GetLogger()), | ||
| user_interface.WithProgressBar(progress.NewTracker(true, engine.GetLogger())))) | ||
| user_interface.WithProgressBar(progress.NewTracker(engine.GetLogger()).New(true)))) |
There was a problem hiding this comment.
AI comment (unchecked by me):
ls_extension GAF UI progress bar also routes to orphaned channel
The same orphaned-channel pattern as the downloader bug exists at
ls_extension/language_server_workflow.go:110.progress.NewTracker(engine.GetLogger()).New(true)creates a standalone Tracker whose channel is never read. The GAF framework callsUpdateProgress/SetTitleon the returned Task, which sends to this channel. In the old code, these events went to the globalToServerProgressChannelwhich the server's listener drained. This is less impactful than the installer path because (a) it only affects the GAF extension entry point and (b) the number of events is likely small (one progress bar for the initial flow). Still, it's worth wiring the per-server owner through to this call site as part of the planned migration documented indocs/requirements/architecture.md:147-148.
User description
Summary
di.TestInitno longer writes package globals —buildTestDependenciesbuilds all services as local variables and returns a self-containedDependenciesstruct, so concurrentTestInitcalls are race-free.progressStopChan— replaced the global 1000-capacity channel with a per-initHandlerschannel (capacity 1, non-blocking send inshutdownHandler), eliminating cross-test stop-signal interference.Installeradded todi.Dependencies— test code usesdeps.Installerrather than the global accessor.di.Xxx()todeps.*—di.ScanStateAggregator(),di.HoverService(),di.Notifier(),di.AuthenticationService(), etc. replaced throughoutapplication/server/.t.Parallel()added to all smoke tests and their subtests — exceptTest_SmokeRealScanMonorepoFixture(usespprof.StartCPUProfile, which is process-global).t.Setenv-after-t.Parallel()panics fixed withos.Setenv+t.Cleanuprestore pattern throughout.browser_noop_test.goguards inapplication/di,application/server,domain/ide/codelens, andinfrastructure/oss— prevent real browser opens in test binaries afterDefaultOpenBrowserFuncwas moved out ofTestInit.Depends on: #1318 (IDE-2103 — fflags data race fix)
What's deferred (not in this PR)
progress.ToServerProgressChannelis still a shared global buffer (documented incleanupChannels) — full isolation requires threading a per-server channel throughNewTracker.command.SetServicewrites a global singleton — documented inTestInitgodoc. Tests exercisingworkspace/executeCommandmust not be markedt.Parallel()until migrated to context injection.Test plan
make test— all unit tests passmake lint— 0 issuesgo test -race ./application/di/...—TestTestInit_ReturnedDepsAreIndependentconfirms independent instancesSMOKE_TESTS=1) —t.Parallel()now added to all shardsPR Type
Enhancement, Bug fix
Description
Remove process-global singletons for testability.
Inject per-server dependencies for concurrency safety.
Integrate server-lifetime contexts for graceful shutdown.
Use per-server progress channels for isolation.
Diagram Walkthrough
flowchart LR A[Old: Process Globals] --> B(New: Per-Server Dependencies) B --> C{Enable t.Parallel()} B --> D{Scoped Contexts (ScanCtx)} B --> E{Per-Server Progress Tracker} F[Tests Pass]File Walkthrough
28 files
Add nolint:gochecknoglobals to package variablesAdd nolint:gochecknoglobals to globalExpandStateAdd nolint:gochecknoglobals to ExpirationInSecondsAdd nolint:gochecknoglobals to analyticsMuAdd nolint:gochecknoglobals to environment variable constantsAdd nolint:gochecknoglobals to codeRendererAdd nolint:gochecknoglobals to issueSeveritiesAdd nolint:gochecknoglobals to style constantsAdd nolint:gochecknoglobals to FlagsAdd nolint:gochecknoglobals to ignorableIacErrorCodesAdd nolint:gochecknoglobals to packageIssueCacheMutexAdd nolint:gochecknoglobals to map constantsAdd nolint:gochecknoglobals to parsedURLStringCacheAdd nolint:gochecknoglobals to ignorableSecretsErrorCodesAdd nolint:gochecknoglobals to initialized flagAdd nolint:gochecknoglobals to ErrorConfig and nonFailingScanErrorsAdd nolint:gochecknoglobals for context keys and remove Clone functionAdd nolint:gochecknoglobals to weightsAdd nolint:gochecknoglobals to FlagsAdd nolint:gochecknoglobals to packageManagerSVGsAdd nolint:gochecknoglobals to DefaultOpenBrowserFuncAdd nolint:gochecknoglobals to map constantsAdd nolint:gochecknoglobals to IssuesSeverityAdd nolint:gochecknoglobals to ldxSyncSettingKeyMapAdd nolint:gochecknoglobals to global variablesAdd nolint:gochecknoglobals to InvalidBranchNameRegexAdd nolint:gochecknoglobals to WORKFLOWID_IDE_DIRECTORY_CHECKAdd nolint:gochecknoglobals to WORKFLOWID_LS45 files
Add test-only init for browser no-opUpdate tests for independent dependencies and parallel safetyRefactor TestInit for race-free, independent dependency creationUse deps.CommandService and deps.ScanCtx in server setupAdd test-only init for browser no-opUse deps.Notifier instead of global di.NotifierUse local dependency instances instead of globalsIsolate API endpoint setting per-engine configTest parallel server setup without os.Setenv racesUpdate tests to inject dependencies into contextUse per-test config isolation and deps for DI accessReplace global FeatureFlagService with local instanceUpdate tests to use per-test dependency instancesTest concurrent CLI runs using real DI depsAdd test config isolation and use DI depsTest per-server ProgressTracker isolationTest ProgressTracker injection and cancel isolationVerify commands use server-lifetime scanCtxTest scanCtx cancellation on server shutdownUpdate tests to use deps for DI access and ProgressTrackerUpdate smoke tests to use WithRealDI and pass depsUse deps for DI access in treeview testsRefactor setupServer to return DI dependencies and use WithAPIEndpointUpdate tests to use deps for DI accessorsAdd test-only init for browser no-opAdd test-only init for browser no-opUpdate tests to use deps for DI access and progress listenersUpdate New call for Scanner to include progress channelUpdate NewService call to pass context.Background()Ensure HandleFolders forwards context to HandleUntrustedFoldersAdd tests for progress channel isolation and context propagationVerify process-global semaphore sharing across executorsTest NewDownloader routes progress to injected owner channelUpdate tests to use per-test progress trackerAdd test progress tracker channel to New callUpdate Scanner constructor calls with progress channelTest progress channel isolation and task self-cancellationAdd test progress tracker channel to New callsAdd test-only init for browser no-opTest NewCLIScanner routes progress to injected owner channelUpdate NewCLIScanner calls with progress tracker channelRemove Clone function due to redundancyTest Tracker cancel and channel isolationUpdate tests for progress channel isolation and new task typeRemove global progress channel cleanup and add per-test tracker17 files
Refactor DI to use local dependencies and per-server contextsUse context-injected CommandService instead of globalRemove global progressStopChan and use per-server channelRefactor DI to use local dependencies and per-server contextsUse server-lifetime scanCtx for background scansInject server-lifetime scanCtx into command factoryInject server-lifetime scanCtx into CommandServiceUse server-lifetime scanCtx for background scan goroutinesInject progress channel into Scanner constructorShare process-global semaphore for CLI concurrencyRefactor Downloader to accept per-server progress ownerInject progress channel into Scanner constructor and use per-taskprogressRefactor progress tracking to per-server owner and taskIntroduce progress channel to IAC scanner and add nolintIntroduce progress channel to CLI scanner and add nolintRefactor progress tracking to per-server owner and taskIntroduce Task as the per-operation progress handle1 files
Fix linting error for reflect.Interface1 files
Add gochecknoglobals linter configuration1 files
Update golanci-lint version to v2.12.21 files
Document progress channel replacement and context injection7 files