fix: add timeout handling to legacy cli #4950

bastiandoetsch · 2023-11-28T09:20:53Z

Pull Request Submission

Please check the boxes once done.

The pull request must:

Reviewer Documentation
- follow CONTRIBUTING rules
- be accompanied by a detailed description of the changes
- contain a risk assessment of the change (Low | Medium | High) with regards to breaking existing functionality. A change e.g. of an underlying language plugin can completely break the functionality for that language, but appearing as only a version change in the dependencies.
- highlight breaking API if applicable
- contain a link to the automatic tests that cover the updated functionality.
- contain testing instructions in case that the reviewer wants to manual verify as well, to add to the manual testing done by the author.
- link to the to the PR for the User-facing documentation
User facing Documentation
- update any relevant documentation in gitbook by submitting a gitbook PR, and including the PR link here
- ensure that the message of the final single commit is descriptive and prefixed with either feat: or fix: , others might be used in rare occasions as well, if there is no need to document the changes in the release notes. The changes or fixes should be described in detail in the commit message for the changelog & release notes.
Testing
- Changes, removals and additions to functionality must be covered by acceptance / integration tests or smoke tests - either already existing ones, or new ones, created by the author of the PR.

Pull Request Review

All pull requests must undergo a thorough review process before being merged.
The review process of the code PR should include code review, testing, and any necessary feedback or revisions.
Pull request reviews of functionality developed in other teams only review the given documentation and test reports.

Manual testing will not be performed by the reviewing team, and is the responsibility of the author of the PR.

For Node projects: It’s important to make sure changes in package.json are also affecting package-lock.json correctly.

If a dependency is not necessary, don’t add it.

When adding a new package as a dependency, make sure that the change is absolutely necessary. We would like to refrain from adding new dependencies when possible.
Documentation PRs in gitbook are reviewed by Snyk's content team. They will also advise on the best phrasing and structuring if needed.

Pull Request Approval

Once a pull request has been reviewed and all necessary revisions have been made, it is approved for merging into
the main codebase. The merging of the code PR is performed by the code owners, the merging of the documentation PR
by our content writers.

What does this PR do?

This PR starts commands in the TypeScript CLI with a deadline, so that timeouts are actually killing spawned sub processes.

Also, the global timeout now waits for a grace-period of 5s on top of the timeout to exit.

Screen.Recording.2023-11-28.at.08.11.46.mov

macbook-basti-lan :: ~/workspace/cli ‹fix/timeout-handling*› » SNYK_CI=1 SNYK_TIMEOUT_SECS=1 binary-releases/snyk-macos test --all-projects -d                              1 ↵
2023-11-28T11:14:31Z main - Version:               1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440
2023-11-28T11:14:31Z main - Platform:              darwin amd64
2023-11-28T11:14:31Z main - API:                   https://api.snyk.io
2023-11-28T11:14:31Z main - Cache:                 /Users/bdoetsch/Library/Caches/snyk/snyk-cli
2023-11-28T11:14:31Z main - Organization:          7d7ab703-3630-4b46-afed-bad53a655ca3
2023-11-28T11:14:31Z main - Insecure HTTPS:        false
2023-11-28T11:14:31Z main - Analytics:             enabled
2023-11-28T11:14:31Z main - Authorization:         6f17318a293de207b1aea1f2052153d1[...]  (type=token)
2023-11-28T11:14:31Z main - Features:
2023-11-28T11:14:31Z main -   oauth:               Disabled
2023-11-28T11:14:31Z main -   fips:                Not available
2023-11-28T11:14:31Z main - Command timeout set for 1 seconds
2023-11-28T11:14:31Z main - Using Legacy CLI to serve the command. (reason: unknown command "test" for "snyk")
2023-11-28T11:14:31Z legacycli:1 - Arguments: [test --all-projects -d]
2023-11-28T11:14:31Z legacycli:1 - Use StdIO: true
2023-11-28T11:14:31Z legacycli:1 - Working directory:
2023-11-28T11:14:31Z legacycli:1 - Init start
2023-11-28T11:14:31Z legacycli:1 - Init-Lock acquired: true (/Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440.lock)
2023-11-28T11:14:31Z legacycli:1 - Validating sha256 of /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/snyk-macos
2023-11-28T11:14:31Z legacycli:1 - open /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/snyk-macos: no such file or directory
2023-11-28T11:14:31Z legacycli:1 - Extract cliv1 to /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/snyk-macos
2023-11-28T11:14:31Z legacycli:1 - Validating sha256 of /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/snyk-macos
2023-11-28T11:14:31Z legacycli:1 - expected:  14a44145b1fe71f5609ae10582bcb360f9c01ef0938ee4a8593fd1d7c5b8eac1
2023-11-28T11:14:31Z legacycli:1 - actual:    14a44145b1fe71f5609ae10582bcb360f9c01ef0938ee4a8593fd1d7c5b8eac1
2023-11-28T11:14:31Z legacycli:1 - Extracted cliv1 successfully
2023-11-28T11:14:31Z legacycli:1 - Init end
2023-11-28T11:14:31Z legacycli:1 - Temporary CertificateLocation: /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/tmp/snyk-cli-cert-1900104616.crt
2023-11-28T11:14:31Z legacycli:1 - Enabled Proxy Authentication Mechanism: Anyauth
2023-11-28T11:14:31Z legacycli:1 - starting proxy
2023-11-28T11:14:31Z legacycli:1 - Wrapper proxy is listening on port:  60154
2023-11-28T11:14:31Z legacycli:1 - Launching:
2023-11-28T11:14:31Z legacycli:1 - /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/snyk-macos
2023-11-28T11:14:31Z legacycli:1 - With Arguments:
2023-11-28T11:14:31Z legacycli:1 - test, --all-projects, -d
2023-11-28T11:14:31Z legacycli:1 - With Environment:
2023-11-28T11:14:31Z legacycli:1 - NODE_EXTRA_CA_CERTS = /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/tmp/snyk-cli-cert-1900104616.crt
2023-11-28T11:14:31Z legacycli:1 - HTTPS_PROXY = http://snykcli:e5d19da8-84ab-4ae4-a756-1ab706dce114@127.0.0.1:60154
2023-11-28T11:14:31Z legacycli:1 - HTTP_PROXY = http://snykcli:e5d19da8-84ab-4ae4-a756-1ab706dce114@127.0.0.1:60154
2023-11-28T11:14:31Z legacycli:1 - NO_PROXY = localhost,127.0.0.1,::1
2023-11-28T11:14:31Z legacycli:1 - SNYK_SYSTEM_HTTPS_PROXY =
2023-11-28T11:14:31Z legacycli:1 - SNYK_SYSTEM_HTTP_PROXY =
2023-11-28T11:14:31Z legacycli:1 - SNYK_SYSTEM_NO_PROXY =
command timed out
2023-11-28T11:14:32Z legacycli:1 - Proxy successfully shut down
2023-11-28T11:14:32Z legacycli:1 - deleting temp cert file: /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/tmp/snyk-cli-cert-1900104616.crt
2023-11-28T11:14:32Z legacycli:1 - deleted temp cert file: /Users/bdoetsch/Library/Caches/snyk/snyk-cli/1.1256.0-dev.9067b9ee7f7407151c23f2bde2742ecc1f0c4440/tmp/snyk-cli-cert-1900104616.crt
2023-11-28T11:14:32Z main - Exiting with 69
2023-11-28T11:14:32Z main - Sending Analytics
2023-11-28T11:14:33Z main - Analytics successfully send

Where should the reviewer start?

cliv2/internal/cliv2/cliv2.go
cliv2/cmd/cliv2/main.go

How should this be manually tested?

Build with make build
SNYK_CI=1 SNYK_TIMEOUT_SECS=1 binary-releases/snyk-macos test --all-projects should exit with exitCode 69 and "command timed out" message

Automatic tests

cli/cliv2/cmd/cliv2/main_test.go

Line 217 in 9067b9e

func Test_setTimeout(t *testing.T) {
cli/cliv2/internal/cliv2/cliv2_test.go

Line 467 in 9067b9e

func Test_setTimeout(t *testing.T) {

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

cmars

Using a context is a huge improvement here and this is a great catch!

Strongly suggest that CLI.PrepareV1Command use exec.CommandContext -- then the context timeout will automatically tear down the node subprocess.

If you do this, it should not be necessary to explicitly check the ctx.Err after running node; this will be handled for you by the os/exec.Cmd, see https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/os/exec/exec.go;l=665

Aside from that, I would strongly recommend checking errors in tests. Discarding them for unconditional stderr printing is OK.

cliv2/internal/cliv2/cliv2_test.go

cliv2/cmd/cliv2/main_test.go

cliv2/cmd/cliv2/main.go

cliv2/internal/cliv2/cliv2.go

Co-authored-by: Casey Marshall <cmars@users.noreply.github.com>

bastiandoetsch · 2023-11-28T14:44:10Z

Using a context is a huge improvement here and this is a great catch!

Strongly suggest that CLI.PrepareV1Command use exec.CommandContext -- then the context timeout will automatically tear down the node subprocess.

If you do this, it should not be necessary to explicitly check the ctx.Err after running node; this will be handled for you by the os/exec.Cmd, see https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/os/exec/exec.go;l=665

Aside from that, I would strongly recommend checking errors in tests. Discarding them for unconditional stderr printing is OK.

Hm. I don't understand what's the difference to the current implementation. PrepareV1Command is already using CommandContext, so what is the suggested change?

cmars

LGTM

cmars · 2023-11-28T14:49:34Z

Hm. I don't understand what's the difference to the current implementation. PrepareV1Command is already using CommandContext, so what is the suggested change?

I missed that change in this branch, sorry!

dotkas

LGTM, agreeing with Casey's comment about https://github.com/snyk/cli/pull/4950/files#r1407783863 though. I usually set Golang to ignore that specific error as I don't agree with it 🤣

bastiandoetsch requested a review from a team as a code owner November 28, 2023 09:20

fix: use deadline context for timeout in legacy workflow

9067b9e

bastiandoetsch force-pushed the fix/timeout-handling branch from 16b8657 to 9067b9e Compare November 28, 2023 09:29

bastiandoetsch assigned cmars Nov 28, 2023

bastiandoetsch added 2 commits November 28, 2023 12:11

fix: tests & refactor error handling for deadline

e18ea8a

fix: display error better

dcc0f70

bastiandoetsch enabled auto-merge (squash) November 28, 2023 11:38

cmars reviewed Nov 28, 2023

View reviewed changes

bastiandoetsch and others added 2 commits November 28, 2023 15:37

chore: use context.WithTimeout

276a176

Co-authored-by: Casey Marshall <cmars@users.noreply.github.com>

chore: assert error is nil

7e8ac78

Co-authored-by: Casey Marshall <cmars@users.noreply.github.com>

cmars approved these changes Nov 28, 2023

View reviewed changes

chore: implement pr suggestions

266e899

bastiandoetsch force-pushed the fix/timeout-handling branch from 61126f6 to 266e899 Compare November 28, 2023 14:56

bastiandoetsch disabled auto-merge November 28, 2023 14:56

fix: test

c59896d

bastiandoetsch enabled auto-merge (squash) November 28, 2023 15:09

dotkas approved these changes Nov 28, 2023

View reviewed changes

bastiandoetsch disabled auto-merge November 28, 2023 15:13

bastiandoetsch merged commit 1686db5 into master Nov 28, 2023
14 checks passed

bastiandoetsch deleted the fix/timeout-handling branch November 28, 2023 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add timeout handling to legacy cli #4950

fix: add timeout handling to legacy cli #4950

bastiandoetsch commented Nov 28, 2023 •

edited

cmars left a comment

bastiandoetsch commented Nov 28, 2023

cmars left a comment

cmars commented Nov 28, 2023

dotkas left a comment

fix: add timeout handling to legacy cli #4950

fix: add timeout handling to legacy cli #4950

Conversation

bastiandoetsch commented Nov 28, 2023 • edited

Pull Request Submission

Pull Request Review

Pull Request Approval

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Automatic tests

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

cmars left a comment

Choose a reason for hiding this comment

bastiandoetsch commented Nov 28, 2023

cmars left a comment

Choose a reason for hiding this comment

cmars commented Nov 28, 2023

dotkas left a comment

Choose a reason for hiding this comment

bastiandoetsch commented Nov 28, 2023 •

edited