-
-
Notifications
You must be signed in to change notification settings - Fork 73
fix: add HTTP client timeout to prevent webhook reports hanging forever #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||
| package github | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "fmt" | ||||||||||||||||
|
|
||||||||||||||||
| gocache "github.com/patrickmn/go-cache" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| func (c *Client) HasReleases(owner, repo string) (bool, error) { | ||||||||||||||||
| cacheKey := "releases:" + owner + "/" + repo | ||||||||||||||||
| if cached, found := c.cache.Get(cacheKey); found { | ||||||||||||||||
| return cached.(bool), nil | ||||||||||||||||
|
Comment on lines
+11
to
+12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe type assertion can panic on cache corruption. The type assertion 🛡️ Proposed fix with safe type assertion- if cached, found := c.cache.Get(cacheKey); found {
- return cached.(bool), nil
+ if cached, found := c.cache.Get(cacheKey); found {
+ if boolVal, ok := cached.(bool); ok {
+ return boolVal, nil
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| v, err, _ := c.sf.Do(cacheKey, func() (interface{}, error) { | ||||||||||||||||
| if cached, found := c.cache.Get(cacheKey); found { | ||||||||||||||||
| return cached.(bool), nil | ||||||||||||||||
|
Comment on lines
+16
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe type assertion can panic on cache corruption. The type assertion 🛡️ Proposed fix with safe type assertion if cached, found := c.cache.Get(cacheKey); found {
- return cached.(bool), nil
+ if boolVal, ok := cached.(bool); ok {
+ return boolVal, nil
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| url := fmt.Sprintf( | ||||||||||||||||
| "https://api.github.com/repos/%s/%s/releases?per_page=1&page=1", | ||||||||||||||||
| owner, repo, | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| var releases []struct{} | ||||||||||||||||
| if err := c.get(url, &releases); err != nil { | ||||||||||||||||
| return false, err | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| hasReleases := len(releases) > 0 | ||||||||||||||||
| c.cache.Set(cacheKey, hasReleases, gocache.DefaultExpiration) | ||||||||||||||||
| return hasReleases, nil | ||||||||||||||||
| }) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return false, err | ||||||||||||||||
| } | ||||||||||||||||
| return v.(bool), nil | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe type assertion can panic if the single-flight function returns unexpected type. Although the single-flight function should always return 🛡️ Proposed fix with safe type assertion- return v.(bool), nil
+ if boolVal, ok := v.(bool); ok {
+ return boolVal, nil
+ }
+ return false, fmt.Errorf("internal error: single-flight returned non-bool value")📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -129,10 +129,13 @@ func (s *Scheduler) executeJob(job config.ScheduledJob) error { | |||||||||||||||||||
| return fmt.Errorf("failed to get contributors: %w", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Check releases | ||||||||||||||||||||
| hasReleases, _ := client.HasReleases(job.Owner, job.Repo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Calculate metrics | ||||||||||||||||||||
| healthScore := analyzer.CalculateHealth(repoInfo, commits) | ||||||||||||||||||||
| busFactor, busRisk := analyzer.BusFactor(contributors) | ||||||||||||||||||||
| maturityScore, maturityLevel := analyzer.RepoMaturityScore(repoInfo, len(commits), len(contributors), false) | ||||||||||||||||||||
| maturityScore, maturityLevel := analyzer.RepoMaturityScore(repoInfo, len(commits), len(contributors), hasReleases) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Build compact config for export | ||||||||||||||||||||
| compactCfg := output.CompactConfig{ | ||||||||||||||||||||
|
|
@@ -285,7 +288,8 @@ func (s *Scheduler) sendToWebhook(webhookURL, filename string, data []byte) erro | |||||||||||||||||||
| return fmt.Errorf("failed to marshal payload: %w", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| resp, err := http.Post(webhookURL, "application/json", bytes.NewBuffer(jsonData)) | ||||||||||||||||||||
| client := &http.Client{Timeout: 30 * time.Second} | ||||||||||||||||||||
| resp, err := client.Post(webhookURL, "application/json", bytes.NewBuffer(jsonData)) | ||||||||||||||||||||
|
Comment on lines
+291
to
+292
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Webhook timeout fix incomplete: missing context cancellation support. The 30-second timeout addresses the core issue from 🔧 Proposed fix using Do() with context client := &http.Client{Timeout: 30 * time.Second}
- resp, err := client.Post(webhookURL, "application/json", bytes.NewBuffer(jsonData))
+ req, err := http.NewRequestWithContext(context.Background(), "POST", webhookURL, bytes.NewBuffer(jsonData))
+ if err != nil {
+ return fmt.Errorf("failed to create webhook request: %w", err)
+ }
+ req.Header.Set("Content-Type", "application/json")
+ resp, err := client.Do(req)If As per static analysis hints, the linter flagged this line with 📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (2.12.2)[error] 292-292: (*net/http.Client).Post must not be called. use (*net/http.Client).Do(*http.Request) (noctx) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| return fmt.Errorf("failed to send webhook: %w", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: agnivo988/Repo-lyzer
Length of output: 490
Add a LICENSE file to the repository root.
The badge on line 8 (
https://github.com/agnivo988/Repo-lyzer/blob/main/LICENSE) references a LICENSE file that does not exist in the repository. This breaks the license badge. Create a LICENSE file in the repository root and choose an appropriate open-source license for the project.The CI workflow badge on line 11 correctly references
.github/workflows/ci.yml, which exists in the repository.🤖 Prompt for AI Agents