add command timeout with context.Cause for better error handling #65
Conversation
📝 WalkthroughWalkthroughReplaces context.WithTimeout with context.WithTimeoutCause in scanning to attach explicit timeout errors; adds HPSM/SCANOSS env handling helpers and a startup test (testHPSMSetup) that validates SCANOSS file-contents access with optional API key and disables HPSM on failure; adds a new config field for the contents API key. Changes
Sequence Diagram(s)sequenceDiagram
participant RunServer
participant EnvHelpers as setupEnvVar* helpers
participant testHPSMSetup
participant SCANOSS as SCANOSS File-Contents API
RunServer->>EnvHelpers: call setupEnvVarFileContentsURL / setupEnvVarFileContents / setupEnvVarHPSMkey
RunServer->>testHPSMSetup: if HPSM enabled -> call testHPSMSetup()
testHPSMSetup->>SCANOSS: HTTP GET <SCANOSS_FILE_CONTENTS_URL>/<known-md5> with optional X-Session (10s timeout)
SCANOSS-->>testHPSMSetup: 2xx or non-2xx / error / timeout
alt 2xx
testHPSMSetup-->>RunServer: success
else non-2xx / error / timeout
testHPSMSetup-->>RunServer: error -> RunServer disables HPSM and logs
end
sequenceDiagram
participant Scanner
participant ContextMgr as Context (WithTimeoutCause)
participant CmdExec as Command Executor
Scanner->>ContextMgr: create ctx via WithTimeoutCause(timeoutErr)
Scanner->>CmdExec: run scan command with ctx
CmdExec-->>Scanner: returns success or error
alt ctx timed out
ContextMgr-->>Scanner: ctx.Cause() == timeoutErr -> log timeout-specific message
else other error
Scanner-->>Scanner: preserve prior error path, include ctx.Cause() when present
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cmd/server.go`:
- Around line 112-117: The code currently logs the sensitive
cfg.Scanning.HPSMcontentsAPIkey when os.Setenv("SCANOSS_API_KEY", ...) fails;
remove the key from logs and only log a non-sensitive message and the error.
Update the failure branch around os.Setenv in the block that checks
cfg.Scanning.HPSMEnabled to call zlog.S (e.g., Infof/Errorf) with a message like
"Failed to set SCANOSS_API_KEY environment variable" and include only the error
(no key value), so references are to cfg.Scanning.HPSMcontentsAPIkey, os.Setenv
and zlog.S.Infof/Errorf for locating the code to change.
🧹 Nitpick comments (2)
pkg/config/server_config.go (1)
70-70: Field naming inconsistency and truncated comment.
- The field name
HPSMcontentsAPIkeyuses inconsistent casing. Following Go conventions for acronyms, considerHPSMContentsAPIKey.- The comment appears truncated:
"HPSM ser"— it seems to be cut off mid-word.Suggested fix
- HPSMcontentsAPIkey string `env:"SCANOSS_FILE_CONTENTS_APIKEY"` // API key used to access the file contents from SCANOSS_FILE_CONTENTS_URL with HPSM ser + HPSMContentsAPIKey string `env:"SCANOSS_FILE_CONTENTS_APIKEY"` // API key used to access the file contents from SCANOSS_FILE_CONTENTS_URL with HPSM serviceNote: If you rename the field, update all usages in
pkg/cmd/server.go(e.g.,cfg.Scanning.HPSMcontentsAPIkey→cfg.Scanning.HPSMContentsAPIKey).pkg/cmd/server.go (1)
175-218: Replacewgetdependency with Go'snet/httpclient for better portability and to fix ineffective stderr capture.While
wgetis available on Alpine Linux by default, using an external command introduces portability risks for Windows, macOS, minimal containers, and other environments withoutwgetpre-installed. Additionally, the-qflag suppresseswget's diagnostic output (including errors sent to stderr), making the stderr capture on lines 207–212 ineffective.The hardcoded MD5
8109a183e06165144dc8d97b791c130fon line 184 should include a comment explaining what file it represents.Using Go's
net/httppackage eliminates the external dependency, improves cross-platform compatibility, and simplifies error handling:Proposed refactor using net/http
func testHPSMSetup(s *zap.SugaredLogger) error { url := os.Getenv("SCANOSS_FILE_CONTENTS_URL") if url == "" { return fmt.Errorf("SCANOSS_FILE_CONTENTS_URL is not set") } if !strings.HasSuffix(url, "/") { url += "/" } // Known test file MD5 (e.g., from a commonly available open-source file) url += "8109a183e06165144dc8d97b791c130f" req, err := http.NewRequest(http.MethodGet, url, nil) if err != nil { return fmt.Errorf("failed to create request: %w", err) } apiKey := os.Getenv("SCANOSS_API_KEY") if apiKey != "" { req.Header.Set("X-Session", apiKey) } else { s.Debug("No SCANOSS_API_KEY set; proceeding without authentication header.") } client := &http.Client{Timeout: 10 * time.Second} s.Debugf("HPSM test request: GET %s", url) resp, err := client.Do(req) if err != nil { s.Debugf("HPSM connection test failed for URL %s: %v", url, err) return err } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { s.Debugf("HPSM connection test failed for URL %s: HTTP %d", url, resp.StatusCode) return fmt.Errorf("HTTP %d", resp.StatusCode) } s.Infof("HPSM setup test successful for URL: %s", url) return nil }This requires adding
"net/http"and"time"imports.
| if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 { | ||
| err := os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey) | ||
| if err != nil { | ||
| zlog.S.Infof("Failed to set SCANOSS_API_KEY value to %v: %v", cfg.Scanning.HPSMcontentsAPIkey, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Security: Avoid logging sensitive API key values.
The API key is logged if os.Setenv fails. Secrets should not appear in logs as they may be persisted and exposed.
Proposed fix
if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 {
err := os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey)
if err != nil {
- zlog.S.Infof("Failed to set SCANOSS_API_KEY value to %v: %v", cfg.Scanning.HPSMcontentsAPIkey, err)
+ zlog.S.Infof("Failed to set SCANOSS_API_KEY: %v", err)
}
}🤖 Prompt for AI Agents
In `@pkg/cmd/server.go` around lines 112 - 117, The code currently logs the
sensitive cfg.Scanning.HPSMcontentsAPIkey when os.Setenv("SCANOSS_API_KEY", ...)
fails; remove the key from logs and only log a non-sensitive message and the
error. Update the failure branch around os.Setenv in the block that checks
cfg.Scanning.HPSMEnabled to call zlog.S (e.g., Infof/Errorf) with a message like
"Failed to set SCANOSS_API_KEY environment variable" and include only the error
(no key value), so references are to cfg.Scanning.HPSMcontentsAPIkey, os.Setenv
and zlog.S.Infof/Errorf for locating the code to change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cmd/server.go`:
- Around line 185-200: Replace the external wget subprocess
(exec.Command("wget", args...)) with a net/http request using http.Client so the
SCANOSS_API_KEY stays in-memory: build an http.NewRequest("GET", url, nil), set
req.Header.Set("X-Session", apiKey) only when apiKey != "", use an http.Client
with Timeout: 10*time.Second, perform client.Do(req) and read/close the response
body, and treat non-2xx status codes as failures (capture status code for
logging). Remove or change s.Debugf("HPSM test command: wget %v", args) to avoid
printing headers/args (log a safe message like "HPSM test request to URL
started" without the key), and delete the args/exec.Command variables (args,
cmd, exec.Command) and any stderr parsing logic that expected wget output;
instead use resp.StatusCode and resp.Body for success/failure handling in the
existing surrounding logic.
♻️ Duplicate comments (1)
pkg/cmd/server.go (1)
112-116: Remove API key from logs.The error log prints
HPSMcontentsAPIkey, which is sensitive and can be persisted in logs. Please redact it.🔧 Proposed fix
if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 { err := os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey) if err != nil { - zlog.S.Infof("Failed to set SCANOSS_API_KEY value to %v: %v", cfg.Scanning.HPSMcontentsAPIkey, err) + zlog.S.Infof("Failed to set SCANOSS_API_KEY: %v", err) } }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cmd/server.go`:
- Around line 78-96: The current code silently ignores errors from os.Setenv
(used directly and in setEnvIfNotEmpty), so modify those sites to check the
error return and surface/log a safe warning (do not print secret values). Update
setEnvIfNotEmpty to return error (or accept a logger) and propagate/check its
error where called (for SCANOSS_API_URL, SCANOSS_FILE_CONTENTS_URL,
SCANOSS_FILE_CONTENTS and SCANOSS_API_KEY), and replace the "_ = os.Setenv(...)"
patterns in the HPSM block and elsewhere with explicit error handling that logs
a non-sensitive warning including the env key and the error (e.g.,
processLogger.Warnf("failed to set env %s: %v", key, err)). Ensure
handleFileContentsFalse() usage remains the same but call it after handling any
Setenv errors.
- Around line 174-209: testHPSMSetup currently uses a hard-coded probe MD5 and
doesn't guard against the special "false" URL, causing false negatives; update
testHPSMSetup to (1) treat SCANOSS_FILE_CONTENTS_URL=="false" as a deliberate
no-op and return nil immediately, (2) read the probe MD5 from a new env var
SCANOSS_PROBE_MD5 with the current literal as a sensible default so deployments
can override it, and (3) consider HTTP 404 from the probe endpoint as a
successful connectivity/authentication check (i.e., do not fail for 404, only
fail on network/errors or non-2xx/non-404 statuses) while keeping the existing
timeout and X-Session header behavior.
pkg/cmd/server.go
Outdated
| setEnvIfNotEmpty("SCANOSS_API_URL", cfg.Scanning.ScanningURL) | ||
|
|
||
| contentsURL := determineContentsURL(cfg) | ||
| setEnvIfNotEmpty("SCANOSS_FILE_CONTENTS_URL", contentsURL) | ||
|
|
||
| _ = os.Setenv("SCANOSS_FILE_CONTENTS", fmt.Sprintf("%v", cfg.Scanning.FileContents)) | ||
|
|
||
| handleFileContentsFalse() | ||
|
|
||
| if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 { | ||
| _ = os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey) | ||
| } | ||
| if len(contentsURL) > 0 { | ||
| err := os.Setenv("SCANOSS_FILE_CONTENTS_URL", contentsURL) | ||
| if err != nil { | ||
| zlog.S.Infof("Failed to set SCANOSS_FILE_CONTENTS_URL value to %v: %v", contentsURL, err) | ||
| } | ||
| } | ||
|
|
||
| func setEnvIfNotEmpty(key, value string) { | ||
| if value != "" { | ||
| _ = os.Setenv(key, value) | ||
| } | ||
| if customContentsURL := os.Getenv("SCANOSS_FILE_CONTENTS_URL"); len(customContentsURL) > 0 { | ||
| zlog.S.Infof("Using custom content URL: %s.", customContentsURL) | ||
| } |
There was a problem hiding this comment.
Don’t silently ignore os.Setenv failures.
If these env vars fail to set, the scan engine/HPSM probe can run with stale defaults and the root cause is hidden. Consider logging a safe warning (no key value) or surfacing the error.
🔧 Suggested hardening
func setupEnvVars(cfg *myconfig.ServerConfig) {
setEnvIfNotEmpty("SCANOSS_API_URL", cfg.Scanning.ScanningURL)
contentsURL := determineContentsURL(cfg)
setEnvIfNotEmpty("SCANOSS_FILE_CONTENTS_URL", contentsURL)
- _ = os.Setenv("SCANOSS_FILE_CONTENTS", fmt.Sprintf("%v", cfg.Scanning.FileContents))
+ setEnvIfNotEmpty("SCANOSS_FILE_CONTENTS", fmt.Sprintf("%v", cfg.Scanning.FileContents))
handleFileContentsFalse()
if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 {
- _ = os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey)
+ setEnvIfNotEmpty("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey)
}
}
func setEnvIfNotEmpty(key, value string) {
if value != "" {
- _ = os.Setenv(key, value)
+ if err := os.Setenv(key, value); err != nil {
+ zlog.S.Warnf("Failed to set %s: %v", key, err)
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setEnvIfNotEmpty("SCANOSS_API_URL", cfg.Scanning.ScanningURL) | |
| contentsURL := determineContentsURL(cfg) | |
| setEnvIfNotEmpty("SCANOSS_FILE_CONTENTS_URL", contentsURL) | |
| _ = os.Setenv("SCANOSS_FILE_CONTENTS", fmt.Sprintf("%v", cfg.Scanning.FileContents)) | |
| handleFileContentsFalse() | |
| if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 { | |
| _ = os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey) | |
| } | |
| if len(contentsURL) > 0 { | |
| err := os.Setenv("SCANOSS_FILE_CONTENTS_URL", contentsURL) | |
| if err != nil { | |
| zlog.S.Infof("Failed to set SCANOSS_FILE_CONTENTS_URL value to %v: %v", contentsURL, err) | |
| } | |
| } | |
| func setEnvIfNotEmpty(key, value string) { | |
| if value != "" { | |
| _ = os.Setenv(key, value) | |
| } | |
| if customContentsURL := os.Getenv("SCANOSS_FILE_CONTENTS_URL"); len(customContentsURL) > 0 { | |
| zlog.S.Infof("Using custom content URL: %s.", customContentsURL) | |
| } | |
| setEnvIfNotEmpty("SCANOSS_API_URL", cfg.Scanning.ScanningURL) | |
| contentsURL := determineContentsURL(cfg) | |
| setEnvIfNotEmpty("SCANOSS_FILE_CONTENTS_URL", contentsURL) | |
| setEnvIfNotEmpty("SCANOSS_FILE_CONTENTS", fmt.Sprintf("%v", cfg.Scanning.FileContents)) | |
| handleFileContentsFalse() | |
| if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 { | |
| setEnvIfNotEmpty("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey) | |
| } | |
| } | |
| func setEnvIfNotEmpty(key, value string) { | |
| if value != "" { | |
| if err := os.Setenv(key, value); err != nil { | |
| zlog.S.Warnf("Failed to set %s: %v", key, err) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/cmd/server.go` around lines 78 - 96, The current code silently ignores
errors from os.Setenv (used directly and in setEnvIfNotEmpty), so modify those
sites to check the error return and surface/log a safe warning (do not print
secret values). Update setEnvIfNotEmpty to return error (or accept a logger) and
propagate/check its error where called (for SCANOSS_API_URL,
SCANOSS_FILE_CONTENTS_URL, SCANOSS_FILE_CONTENTS and SCANOSS_API_KEY), and
replace the "_ = os.Setenv(...)" patterns in the HPSM block and elsewhere with
explicit error handling that logs a non-sensitive warning including the env key
and the error (e.g., processLogger.Warnf("failed to set env %s: %v", key, err)).
Ensure handleFileContentsFalse() usage remains the same but call it after
handling any Setenv errors.
| func testHPSMSetup(s *zap.SugaredLogger) error { | ||
| url := os.Getenv("SCANOSS_FILE_CONTENTS_URL") | ||
| if url == "" { | ||
| return fmt.Errorf("SCANOSS_FILE_CONTENTS_URL is not set") | ||
| } | ||
| // Ensure URL ends with "/" before appending the test MD5 | ||
| url = strings.TrimSuffix(url, "/") + "/8109a183e06165144dc8d97b791c130f" | ||
|
|
||
| s.Debug("HPSM test request started") | ||
|
|
||
| // Create HTTP GET request | ||
| req, err := http.NewRequest(http.MethodGet, url, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create HPSM test request: %w", err) | ||
| } | ||
|
|
||
| // Set X-Session header if API key is present | ||
| if apiKey := os.Getenv("SCANOSS_API_KEY"); apiKey != "" { | ||
| req.Header.Set("X-Session", apiKey) | ||
| } | ||
|
|
||
| // Perform the request with 10 second timeout | ||
| client := &http.Client{Timeout: 10 * time.Second} | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("HPSM connection test failed: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Read and discard the response body | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
|
|
||
| // Treat non-2xx status codes as failures | ||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| return fmt.Errorf("HTTP status %d", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual code at the specified lines
cat -n pkg/cmd/server.go | sed -n '174,209p'Repository: scanoss/api.go
Length of output: 1443
🏁 Script executed:
# Search for the function name to ensure we're looking at the right code
rg "testHPSMSetup" -A 40 pkg/cmd/server.goRepository: scanoss/api.go
Length of output: 1537
🏁 Script executed:
# Search for handleFileContentsFalse to understand the "false" URL reference
rg "handleFileContentsFalse|false" pkg/cmd/server.goRepository: scanoss/api.go
Length of output: 408
🏁 Script executed:
# Search for SCANOSS_FILE_CONTENTS_URL usage across the codebase
rg "SCANOSS_FILE_CONTENTS_URL" --type goRepository: scanoss/api.go
Length of output: 829
🏁 Script executed:
# Search for other probe MD5 references or configuration
rg "8109a183e06165144dc8d97b791c130f|probe.*MD5|MD5.*probe" --type goRepository: scanoss/api.go
Length of output: 149
Hard-coded probe MD5 can yield false negatives; "false" URL and missing probe need explicit handling.
The code hard-codes a probe MD5 (line 180: 8109a183e06165144dc8d97b791c130f) and only validates that SCANOSS_FILE_CONTENTS_URL is non-empty (line 176). While handleFileContentsFalse() sets the URL to "false" and also disables HPSM in that function, testHPSMSetup itself has no guard against a "false" URL string—any caller that bypasses handleFileContentsFalse() will fail with a bad request. More critically, if the probe MD5 does not exist in a deployment, the endpoint returns 404, which disables HPSM even when connectivity and authentication are working correctly.
Either (1) ensure the probe MD5 always exists across all deployments, (2) treat 404 as connectivity success, or (3) make the probe MD5 configurable.
🛠️ Suggested fixes
url := os.Getenv("SCANOSS_FILE_CONTENTS_URL")
-if url == "" {
- return fmt.Errorf("SCANOSS_FILE_CONTENTS_URL is not set")
-}
+if url == "" || strings.EqualFold(url, "false") {
+ return fmt.Errorf("SCANOSS_FILE_CONTENTS_URL is disabled")
+}
...
-// Treat non-2xx status codes as failures
-if resp.StatusCode < 200 || resp.StatusCode >= 300 {
- return fmt.Errorf("HTTP status %d", resp.StatusCode)
-}
+if resp.StatusCode == http.StatusNotFound {
+ s.Debug("HPSM probe MD5 not found; treating connectivity as OK")
+ return nil
+}
+if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+ return fmt.Errorf("HTTP status %d", resp.StatusCode)
+}🤖 Prompt for AI Agents
In `@pkg/cmd/server.go` around lines 174 - 209, testHPSMSetup currently uses a
hard-coded probe MD5 and doesn't guard against the special "false" URL, causing
false negatives; update testHPSMSetup to (1) treat
SCANOSS_FILE_CONTENTS_URL=="false" as a deliberate no-op and return nil
immediately, (2) read the probe MD5 from a new env var SCANOSS_PROBE_MD5 with
the current literal as a sensible default so deployments can override it, and
(3) consider HTTP 404 from the probe endpoint as a successful
connectivity/authentication check (i.e., do not fail for 404, only fail on
network/errors or non-2xx/non-404 statuses) while keeping the existing timeout
and X-Session header behavior.
There was a problem hiding this comment.
this test function will be only executed if this service functionality is enabled, so the function to setup and validate the env-vars was already executed at this point. Also, this file will be always in the KB,=
There was a problem hiding this comment.
@mscasso-scanoss, thank you for the clarification! You're absolutely right—since testHPSMSetup only runs after setupEnvVars(cfg) has already been called, and only when HPSMEnabled is true, the environment variables will already be properly configured at that point. The "false" URL case is handled by handleFileContentsFalse() before this test runs, so that concern doesn't apply here.
I appreciate you pointing out the execution context—I should have considered the full flow before raising the concern. My apologies for the noise! 🐰
/\_/\
( o.o )
> ^ <
"Understood!"
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.