fix: server lifecycle context [IDE-1791]

[rrama]

Description

Done by passing a context to the loading of the configured environment.
This will prevent interactive terminals being launched when the server shutsdown.
Currently targeting the PR branch in GAF (snyk/go-application-framework#564). Change before merge.

Checklist

• Tests added and all succeed
• Regenerated mocks, etc. (make generate)
• N/A
• Linted (make lint-fix)
• README.md updated, if user-facing
• N/A
• License file updated, if new 3rd-party dependency is introduced
• N/A

[snyk-io]

✅ 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-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Risk of Process Crash 🟠 [major] The ServerLifecycleContext and CancelServerLifecycleContext functions now include hard panic calls if the server lifecycle context is not initialized. Since the Config object is a core dependency passed throughout the codebase and instantiated in various contexts (including utility tools or tests that may not go through the full server.Start() or testutil.UnitTestWithCtx() flow), this introduces a risk of unintended process crashes. Consider returning context.Background() or a no-op function with a warning log as a fallback instead of panicking. if c.serverLifecycleCtx == nil { panic("server lifecycle context is not initialized") } Hard Panic on Initialization 🟡 [minor] The SetServerLifecycleContext function panics if either the context or the cancel function is nil. While ensuring initialization is important, a panic in a setter for a configuration object is an extreme way to handle validation. Using an error return or logging a critical error would be more robust. if ctx == nil { panic("server lifecycle context must not be nil") }

📚 Repository Context Analyzed This review considered 47 relevant code sections from 13 files (average relevance: 1.05) application/config/config.go (2 segments, lines 903-1721) domain/snyk/scanner/scanner.go (lines 1-429) application/server/server.go (2 segments, lines 62-721) application/di/init.go (lines 1-278) infrastructure/code/code_instrumentor.go (lines 1-107) domain/ide/command/update_folder_config.go (lines 1-193) domain/ide/workspace/workspace.go (lines 1-286) ... and 6 more file(s)

[bastiandoetsch]

We need clone here as we have a deferred cancel of ctx in L226. This means, we need to decouple the reference scan context from the parent, as the reference scan is running async.

[rrama]

I spoke with Basti, I am going to wait for his big LS changes, then I will jump on his context enriching in Start and I will have the lifecycle context be fetchable from the enriched context.

[bastiandoetsch]

[Should Fix] These accessors panic when the lifecycle context was never set, but newConfig doesn't default-initialize it — only server.Start() and the test helpers do. Production consumers reached deep in background paths (internal/sdk/sdk.go, infrastructure/oss/cli_scanner.go:602 scheduleRefreshScan) call ServerLifecycleContext() unconditionally, so any Config built via config.New/NewFromExtension that triggers a scan without going through Start() (an embedder, or a unit test bypassing the testutil helpers) hard-panics the whole server instead of degrading. Flagged by 3 of 4 reviewers.

Fix: default-initialize the field in newConfig with context.WithCancel(context.Background()) (overwritten by Start), which lets you delete all three panic guards. (Storing a context on the long-lived shared Config singleton is also the containedctx anti-pattern — pragmatic here since Config is the universal handle, but worth a comment.)

— AI review

[bastiandoetsch]

[Should Fix · verify] Binding the reference-branch scan to the parent ctx (instead of the previous Clone(ctx, context.Background()) decoupling) is the right call for shutdown cancellation, but it newly makes the base scan abortable by parent-context cancellation. The base scan does a git checkout (scanBaseBranch → gitCheckoutHandler); if ctx is cancelled mid-checkout the working tree could be left on the reference branch, which was previously impossible. Please confirm every Scan caller passes a lifecycle-bound ctx (not a per-message ctx that cancels on the next request), and that scanBaseBranch restores the branch on cancellation. (3 of 4 reviewers raised this; the WaitGroup makes the normal-completion path safe, the concern is the cancel path.)

— AI review

[bastiandoetsch]

A couple more items from the verification pass (test files; not anchoring inline to keep it readable):

[Should Fix] application/server/server_test.go — setupServer registers the shutdown/exit t.Cleanup before di.TestInit(t). t.Cleanup is LIFO, so the gomock controller's Finish (registered later) now runs before the shutdown/exit handlers, where the old setupCustomServer ran shutdown while mocks were still live. Any mock interaction or expectation reached on the shutdown path now risks an order-dependent 'missing call'/'controller finished' failure. Register the shutdown/exit cleanup after di.TestInit (or otherwise restore the original teardown order).

[Suggestion] server_test.go (~line 142) — assert.NoError(t, exitCtx.Err(), "LS exit took too long") runs right after a fire-and-forget Notify, which returns before the exit handler completes, so exitCtx.Err() is essentially always nil — the assertion can't catch a stuck exit. Consider waiting on an actual completion signal.

[Suggestion] Two files solve 'decouple but cancel on shutdown' with different idioms (Clone(ctx, ServerLifecycleContext()) in cli_scanner.go vs using ctx directly in scanner.go) — worth a single shared helper.

Nice catch on the side: infrastructure/cli/cli.go — the refactor fixes a real latent bug, the old Trace().Str("response", ...) had no terminating .Msg()/.Send() and never emitted. Security scan: clean, and the GAF bump was verified to introduce zero new vulns (its go.mod hash is unchanged).

— AI review

[basti-snyk]

Worth confirming — semantic change to reference-branch scan cancellation. Previously this cloned onto a fresh context.Background() (ctx2.Clone(ctx, context.Background())), deliberately decoupling the base/reference scan from the parent's cancellation so it always ran to completion and persisted its baseline snapshot. Deriving directly from ctx now places the reference scan in the parent's cancellation tree.

For the shutdown case this is the intended improvement (no detached goroutine). But there's a narrower regression to check: if the parent scan ctx is cancelled by a token change mid-scan (the auth-token-change cancel path), the in-flight base scan now aborts before persistScanResults, so no baseline is persisted and the next delta scan has no baseline — where the old decoupled clone guaranteed completion.

The goroutine lifetime itself is fine (bounded by referenceBranchScanWaitGroup.Wait()). Please confirm aborting the baseline on a token-change cancel is intended. Also worth a comment: cli_scanner.go clones its scheduled-scan base onto the lifecycle ctx, so the two scanners now use different base-ctx strategies — note it so they don't drift.

— AI review

[basti-snyk]

ℹ️ Automated AI review summary — non-blocking; one item to confirm is inline.

Reviewed origin/main...HEAD with four reviewers. Verdict: the lifecycle-context rework is sound and achieves its intent for the production single-Start path. Several initially-high-severity hypotheses were independently refuted on closer inspection and are NOT issues:

• ServerLifecycleContext() panicking in unit tests / scheduleRefreshScan — refuted: testutil.UnitTest delegates to UnitTestWithCtx which sets the context; the touched packages build and the panic-path tests pass.
• Start's defer lifecycleCancel() cancelling while the server is alive — refuted: Start blocks on srv.WaitStatus(), so the defer is a post-exit safety net.
• The --about/licenses path panicking — refuted: it uses the passed-in context.Background(), never the lifecycle getter.

codeaction_handlers.go cleanly fixes a real leaked-context (govet lostcancel); the context field is RWMutex-guarded; the dep bump introduces zero new vulnerabilities.

Worth confirming (inline on scanner.go:301): dropping the ctx2.Clone(ctx, context.Background()) decoupling means a token-change-mid-scan now aborts the reference-branch base scan before its baseline persists. Correct for shutdown, but please confirm it's intended for the token-change path.

Suggestions (non-blocking):

• The panic-based ServerLifecycleContext() / CancelServerLifecycleContext() contract is fail-fast but fragile for any future non-server entrypoint; consider a logged context.Background() fallback / no-op cancel.
• cli.go bundles unrelated refactors into this PR. Keep the genuine .Msg() fix (the prior Trace().Str(...) logged nothing), but the logger refactor is scope creep — consider splitting.
• Document the single-Start-per-Config lifetime assumption (or reset the field on shutdown) so reuse across start/stop cycles is safe.
• Minor: rationale comment is duplicated across three handlers vs the method doc; the new setupServer cleanup adds a full shutdown+exit handshake to ~87 tests (watch suite latency); options scaffolding is heavy for two booleans.

Security: no findings introduced; the 60 pre-existing SCA vulns are unchanged by the dep bump and have no available fix.

— AI review