chore: Send instrumentation data on kill

[PeterSchafer]

Pull Request Submission Checklist

• Follows CONTRIBUTING guidelines
• Commit messages are release-note ready, emphasizing what was changed, not how.
• Includes detailed description of changes
• Contains risk assessment (Low | Medium | High)
• Highlights breaking API changes (if applicable)
• Links to automated tests covering new functionality
• Includes manual testing instructions (if necessary)
• Updates relevant GitBook documentation (PR link: ___)
• Includes product update to be communicated in the next stable release notes

What does this PR do?

Ensures instrumentation data is sent when the CLI is terminated by a signal (SIGINT/SIGTERM). Previously, if a user killed a running CLI command (e.g., via Ctrl+C), instrumentation data would be lost. This change:

1. Refactors teardown logic into a dedicated tearDown() function that handles analytics, instrumentation, cleanup, and logging
2. Adds signal handling (behind PREVIEW_FEATURES_ENABLED flag) that intercepts SIGINT/SIGTERM and runs teardown before exiting
3. Uses sync.Once to ensure teardown runs exactly once, whether triggered by normal completion or signal
4. Extracts instrumentation helpers to a new instrumentation.go file for better code organization
5. Updates error-catalog dependency to include NewTerminatedBySignalError for proper error categorization

Where should the reviewer start?

Start with cliv2/cmd/cliv2/main.go:487-523 for the new tearDown() function, then review the signal handling setup at lines 635-656.

snyk/go-application-framework#578

How should this be manually tested?

with a developer build

1. Run a long-running command (e.g., snyk test on a large project)
2. Press Ctrl+C to send SIGINT
3. Verify in debug logs (--debug) that instrumentation is sent before exit

What's the product update that needs to be communicated to CLI users?

N/A - This is an internal improvement to telemetry reliability. No user-facing behavior changes.

Risk assessment (Low | Medium | High)?

Medium - Changes core CLI exit flow, but:

• Signal handling is gated behind PREVIEW_FEATURES_ENABLED flag
• Uses sync.Once to prevent double-execution
• Signal handler is stopped before normal teardown to prevent race conditions

Any background context you want to provide?

When users terminate CLI commands prematurely, we lose visibility into those interactions. This change improves our ability to understand CLI usage patterns and identify issues with long-running commands.

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

[j-luong]

question: if teardown hangs (e.g. when making the analytics API call), can we still SIGINT the process? Or does teardownOnce prevent this? Basically, is it possible to be a hanging state?

[PeterSchafer]

As mentioned before, I'm working on a solution.

[j-luong]

question(0): what's the reason for putting this behind preview features?
question(1): what's the condition for removing this?

[PeterSchafer]

Q1: This could easily have side effects that I want to carefully roll out. Using preview only is a simple straight forward way to collect feedback from internal and preview usage in general.
Q2: That is a good question. I would say if we think it creates value and if concerns are mitigated.

[j-luong]

suggestion: is it possible to write a test to validate this behaviour?

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Instrumentation Suppression on Signal 🟠 [major] The tearDown function initializes teardownCtx using context.WithTimeout(ctx, teardownTimeout). Since ctx is the main context cancelled by the signal handler (line 655) before tearDown is called (line 664), the resulting teardownCtx is also immediately cancelled. As a result, sendAnalytics and sendInstrumentation will fail to send any data because their underlying HTTP requests or engine invocations will exit immediately with a context canceled error. To fix this, the teardown phase should use a fresh context (e.g., context.WithoutCancel(ctx) or context.Background()) for IO operations. teardownCtx, cancel := context.WithTimeout(ctx, teardownTimeout) defer cancel() Goroutine Leak 🟡 [minor] The signal handling goroutine (lines 650-669) blocks on <-signalChan. On normal CLI exit, signal.Stop(signalChan) is called (line 710), which stops future signals but does not close the channel. The goroutine will remain blocked indefinitely. While minor for a short-lived CLI, the channel should be closed or the goroutine should select on the main context to exit gracefully. sig := <-signalChan

📚 Repository Context Analyzed This review considered 31 relevant code sections from 13 files (average relevance: 0.91) src/cli/main.ts (lines 1-235) cliv2/cmd/cliv2/debug.go (lines 1-73) cliv2/cmd/cliv2/logheaderfooter.go (lines 1-254) cliv2/cmd/cliv2/main.go (lines 205-406) scripts/upgrade-snyk-go-dependencies.go (lines 1-135) cliv2/cmd/cliv2/exitcode.go (lines 1-197) cliv2/internal/constants/constants.go (lines 1-43) ... and 6 more file(s)

[danskmt]

The context ctx here can happen to be already-cancelled as it is derived from the parent context. Testing locally, I saw this issue happening:

Here I suppose you should use a fresh context for tearing down:

teardownCtx, cancel := context.WithTimeout(context.Background(), teardownTimeout)

Also, the context parameter we can use ctxCancel, updating the function signature:
func tearDown(ctx context.Context, ... -> func tearDown(ctxCancel context.CancelFunc, ...

in the calls of tearDown (L664 and L716), we provide ctxCancel:

finalExitCode = tearDown(ctxCancel, signalError, errorListCopy, startTime, ua, cliAnalytics, networkAccess)
...
finalExitCode = tearDown(ctxCancel, err, errorListCopy, startTime, ua, cliAnalytics, networkAccess)

And after sendInstrumentation call (line 516) can terminate child processes:

sendInstrumentation(teardownCtx, globalEngine, cliAnalytics.GetInstrumentation(), globalLogger)
ctxCancel()

i.e. instead of calling ctxCancel in line 655, we do it here after sendInstrumentation, wdyt?

[PeterSchafer]

Thanks @danskmt ! Good catches! I marked the PR as draft as it is far from ready. There are plenty of local changes as well, so I prematurely marked this as to review. Sorry for that!

[danskmt]

No worries! Let me know when you want me to review it again.