fix: add HTTP client timeout to prevent webhook reports hanging forever#484
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesHasReleases integration and webhook timeout fix
README documentation update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (4)
cmd/analyze.go (1)
318-319: ⚡ Quick winConsider logging the HasReleases error.
The error from
HasReleasesis silently discarded. While defaulting tohasReleases = falseis safe, logging the error would help diagnose API issues or rate limit problems.📝 Proposed logging
// Check releases - hasReleases, _ := client.HasReleases(owner, repo) + hasReleases, err := client.HasReleases(owner, repo) + if err != nil { + log.Printf("Failed to check releases for %s/%s: %v", owner, repo, err) + hasReleases = false + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/analyze.go` around lines 318 - 319, The error returned by the HasReleases method call is being silently discarded with the blank identifier. Capture this error return value and log it using an appropriate logger (debug or warn level) to help diagnose API connectivity issues or rate limit problems, while still allowing the code to safely default hasReleases to false when the call fails.internal/ui/app.go (1)
1433-1434: ⚡ Quick winConsider logging HasReleases errors in comparison flow.
Both
HasReleasescalls silently discard errors. While defaulting tofalseis safe, logging would help diagnose API issues during repository comparisons.📝 Proposed logging
busFactor1, busRisk1 := analyzer.BusFactor(contributors1) - hasReleases1, _ := client.HasReleases(parts1[0], parts1[1]) + hasReleases1, err := client.HasReleases(parts1[0], parts1[1]) + if err != nil { + log.Printf("Failed to check releases for %s: %v", repo1Name, err) + hasReleases1 = false + } maturityScore1, maturityLevel1 := analyzer.RepoMaturityScore(repo1, len(commits1), len(contributors1), hasReleases1)Apply the same pattern for
hasReleases2at line 1460.Also applies to: 1460-1461
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/app.go` around lines 1433 - 1434, The HasReleases method calls are silently discarding errors using the blank identifier, which prevents visibility into API issues during repository comparisons. In the comparison flow in internal/ui/app.go, capture the error returned by both HasReleases calls (at lines 1433-1434 for hasReleases1 and at lines 1460-1461 for hasReleases2) and log them appropriately instead of using the blank identifier to discard them. This will help diagnose API failures while maintaining the safe default behavior of treating missing releases as false.internal/scheduler/scheduler.go (1)
132-133: ⚡ Quick winConsider logging the HasReleases error.
The error from
HasReleasesis silently discarded. While defaulting tohasReleases = falseis safe, logging the error would help diagnose API issues, especially important in scheduled jobs where errors might go unnoticed.📝 Proposed logging
// Check releases - hasReleases, _ := client.HasReleases(job.Owner, job.Repo) + hasReleases, err := client.HasReleases(job.Owner, job.Repo) + if err != nil { + log.Printf("Failed to check releases for %s/%s in job %s: %v", job.Owner, job.Repo, job.ID, err) + hasReleases = false + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduler/scheduler.go` around lines 132 - 133, The error returned by the HasReleases method call in the scheduler is being silently discarded with a blank identifier. Instead of ignoring the error, capture it in a named variable and add conditional logging to record the error if it occurs. This will help diagnose API issues in scheduled jobs without changing the safe default behavior of setting hasReleases to false when an error occurs. Update the line where HasReleases is called to capture both the boolean result and the error, then add a log statement that checks if the error is not nil and logs the error details.internal/analyzer/certificate.go (1)
140-141: ⚡ Quick winConsider logging the HasReleases error.
The error from
HasReleasesis silently discarded. While defaulting tohasReleases = falseis safe, logging the error would help diagnose API issues.📝 Proposed logging
// Check releases - hasReleases, _ := client.HasReleases(owner, repo) + hasReleases, err := client.HasReleases(owner, repo) + if err != nil { + log.Printf("Failed to check releases for %s/%s: %v", owner, repo, err) + hasReleases = false + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/analyzer/certificate.go` around lines 140 - 141, The error returned by the HasReleases call on the client is being silently discarded using an underscore. Instead of discarding the error, capture it into a named variable and log it if an error occurs. This way, the hasReleases variable will still safely default to false if the method fails, but any API issues or other errors will be logged for debugging purposes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/github/releases.go`:
- Line 37: The type assertion at line 37 in the releases.go file is unsafe and
can cause a panic if the single-flight function returns an unexpected type.
Replace the unsafe type assertion `v.(bool), nil` with a safe type assertion
using the two-return-value form to check whether the type assertion succeeded.
If the type assertion fails, return an appropriate error to handle the
unexpected type gracefully instead of allowing a panic.
- Around line 16-17: The type assertion `cached.(bool)` in the cache retrieval
block will panic if the cached value is not a boolean. Replace the unsafe type
assertion with the safe type assertion pattern using the comma-ok idiom (e.g.,
`value, ok := cached.(bool)`). Check the ok flag and handle the case where the
type assertion fails, either by returning an error or logging a warning and
falling through to re-fetch the value from the source.
- Around line 11-12: The type assertion `cached.(bool)` in the cache retrieval
code is unsafe and will cause a panic if the cached value is not actually a
boolean type. Replace the unsafe type assertion with a safe type assertion using
the comma-ok idiom (e.g., `ok := true; if v, ok := cached.(bool); ok { ... }`).
This way, if the cache value is corrupted or has an unexpected type, the code
will handle it gracefully instead of panicking, and you can either return an
error or treat it as a cache miss.
In `@internal/scheduler/scheduler.go`:
- Around line 291-292: The current implementation using `client.Post()` doesn't
support context cancellation, which means webhook requests cannot be interrupted
during graceful shutdown or job cancellation. Replace the direct `client.Post()`
call with `http.NewRequestWithContext()` to create an `*http.Request` with the
appropriate context (likely from the `executeJob` function's parameters), then
call `client.Do()` with that request instead. This enables the HTTP call to
respect context cancellation while maintaining the 30-second timeout already
configured on the http.Client.
In `@README.md`:
- Around line 6-11: The license badge in README.md references a LICENSE file at
the repository root that does not currently exist, causing the badge to break.
Create a LICENSE file in the repository root directory and select an appropriate
open-source license for the project. The CI workflow badge already correctly
references the existing .github/workflows/ci.yml file, so no changes are needed
for that badge.
---
Nitpick comments:
In `@cmd/analyze.go`:
- Around line 318-319: The error returned by the HasReleases method call is
being silently discarded with the blank identifier. Capture this error return
value and log it using an appropriate logger (debug or warn level) to help
diagnose API connectivity issues or rate limit problems, while still allowing
the code to safely default hasReleases to false when the call fails.
In `@internal/analyzer/certificate.go`:
- Around line 140-141: The error returned by the HasReleases call on the client
is being silently discarded using an underscore. Instead of discarding the
error, capture it into a named variable and log it if an error occurs. This way,
the hasReleases variable will still safely default to false if the method fails,
but any API issues or other errors will be logged for debugging purposes.
In `@internal/scheduler/scheduler.go`:
- Around line 132-133: The error returned by the HasReleases method call in the
scheduler is being silently discarded with a blank identifier. Instead of
ignoring the error, capture it in a named variable and add conditional logging
to record the error if it occurs. This will help diagnose API issues in
scheduled jobs without changing the safe default behavior of setting hasReleases
to false when an error occurs. Update the line where HasReleases is called to
capture both the boolean result and the error, then add a log statement that
checks if the error is not nil and logs the error details.
In `@internal/ui/app.go`:
- Around line 1433-1434: The HasReleases method calls are silently discarding
errors using the blank identifier, which prevents visibility into API issues
during repository comparisons. In the comparison flow in internal/ui/app.go,
capture the error returned by both HasReleases calls (at lines 1433-1434 for
hasReleases1 and at lines 1460-1461 for hasReleases2) and log them appropriately
instead of using the blank identifier to discard them. This will help diagnose
API failures while maintaining the safe default behavior of treating missing
releases as false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f4eb136-d9f2-4ea2-9e7f-47c08048fc09
📒 Files selected for processing (8)
README.mdcmd/analyze.gocmd/compare.gointernal/analyzer/certificate.gointernal/github/releases.gointernal/output/compare.gointernal/scheduler/scheduler.gointernal/ui/app.go
| if cached, found := c.cache.Get(cacheKey); found { | ||
| return cached.(bool), nil |
There was a problem hiding this comment.
Unsafe type assertion can panic on cache corruption.
The type assertion cached.(bool) will panic if the cached value is not a boolean (for example, due to cache corruption or a programming error elsewhere that stores a different type under this key).
🛡️ 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
‼️ 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.
| 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 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/github/releases.go` around lines 11 - 12, The type assertion
`cached.(bool)` in the cache retrieval code is unsafe and will cause a panic if
the cached value is not actually a boolean type. Replace the unsafe type
assertion with a safe type assertion using the comma-ok idiom (e.g., `ok :=
true; if v, ok := cached.(bool); ok { ... }`). This way, if the cache value is
corrupted or has an unexpected type, the code will handle it gracefully instead
of panicking, and you can either return an error or treat it as a cache miss.
| if cached, found := c.cache.Get(cacheKey); found { | ||
| return cached.(bool), nil |
There was a problem hiding this comment.
Unsafe type assertion can panic on cache corruption.
The type assertion cached.(bool) will panic if the cached value is not a boolean. Apply the same safe type assertion pattern as the outer cache check.
🛡️ 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
‼️ 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.
| 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 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/github/releases.go` around lines 16 - 17, The type assertion
`cached.(bool)` in the cache retrieval block will panic if the cached value is
not a boolean. Replace the unsafe type assertion with the safe type assertion
pattern using the comma-ok idiom (e.g., `value, ok := cached.(bool)`). Check the
ok flag and handle the case where the type assertion fails, either by returning
an error or logging a warning and falling through to re-fetch the value from the
source.
| if err != nil { | ||
| return false, err | ||
| } | ||
| return v.(bool), nil |
There was a problem hiding this comment.
Unsafe type assertion can panic if the single-flight function returns unexpected type.
Although the single-flight function should always return bool, defensive programming suggests using a safe type assertion here as well.
🛡️ 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
‼️ 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.
| 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") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/github/releases.go` at line 37, The type assertion at line 37 in the
releases.go file is unsafe and can cause a panic if the single-flight function
returns an unexpected type. Replace the unsafe type assertion `v.(bool), nil`
with a safe type assertion using the two-return-value form to check whether the
type assertion succeeded. If the type assertion fails, return an appropriate
error to handle the unexpected type gracefully instead of allowing a panic.
| client := &http.Client{Timeout: 30 * time.Second} | ||
| resp, err := client.Post(webhookURL, "application/json", bytes.NewBuffer(jsonData)) |
There was a problem hiding this comment.
Webhook timeout fix incomplete: missing context cancellation support.
The 30-second timeout addresses the core issue from #391 (preventing indefinite hangs), but the implementation using client.Post() doesn't respect context cancellation. If the scheduler needs to shut down gracefully or cancel a job, the webhook delivery will still block for up to 30 seconds.
🔧 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 executeJob should support context cancellation (which scheduled jobs typically should), pass that context instead of context.Background().
As per static analysis hints, the linter flagged this line with noctx - HTTP calls should use (*http.Client).Do(*http.Request) with context for proper cancellation support.
📝 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.
| client := &http.Client{Timeout: 30 * time.Second} | |
| resp, err := client.Post(webhookURL, "application/json", bytes.NewBuffer(jsonData)) | |
| client := &http.Client{Timeout: 30 * time.Second} | |
| 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) |
🧰 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 Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scheduler/scheduler.go` around lines 291 - 292, The current
implementation using `client.Post()` doesn't support context cancellation, which
means webhook requests cannot be interrupted during graceful shutdown or job
cancellation. Replace the direct `client.Post()` call with
`http.NewRequestWithContext()` to create an `*http.Request` with the appropriate
context (likely from the `executeJob` function's parameters), then call
`client.Do()` with that request instead. This enables the HTTP call to respect
context cancellation while maintaining the 30-second timeout already configured
on the http.Client.
Source: Linters/SAST tools
| <p align="center"> | ||
| <a href="https://github.com/agnivo988/Repo-lyzer/releases"><img src="https://img.shields.io/github/v/release/agnivo988/Repo-lyzer?style=flat-square" alt="Release"></a> | ||
| <a href="https://github.com/agnivo988/Repo-lyzer/blob/main/LICENSE"><img src="https://img.shields.io/github/license/agnivo988/Repo-lyzer?style=flat-square" alt="License"></a> | ||
| <a href="https://github.com/agnivo988/Repo-lyzer/issues"><img src="https://img.shields.io/github/issues/agnivo988/Repo-lyzer?style=flat-square" alt="Issues"></a> | ||
| <a href="https://github.com/agnivo988/Repo-lyzer/actions"><img src="https://img.shields.io/github/actions/workflow/status/agnivo988/Repo-lyzer/ci.yml?style=flat-square" alt="CI"></a> | ||
| </p> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that README badge URLs point to valid targets.
# Check 1: Verify LICENSE file exists in repository root
echo "=== Checking LICENSE file ==="
if [ -f "LICENSE" ]; then
echo "✓ LICENSE file exists"
else
echo "✗ LICENSE file not found"
fi
# Check 2: Verify CI workflow file matches badge reference
echo ""
echo "=== Checking CI workflow ==="
if [ -f ".github/workflows/ci.yml" ]; then
echo "✓ CI workflow (.github/workflows/ci.yml) exists"
else
echo "✗ CI workflow not found at .github/workflows/ci.yml"
fi
# Check 3: List GitHub Actions workflows to confirm the correct workflow name
echo ""
echo "=== Available GitHub Actions workflows ==="
if [ -d ".github/workflows" ]; then
ls -la .github/workflows/ | grep -E "\.yml|\.yaml"
else
echo "No .github/workflows directory found"
fiRepository: 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 6 - 11, The license badge in README.md references a
LICENSE file at the repository root that does not currently exist, causing the
badge to break. Create a LICENSE file in the repository root directory and
select an appropriate open-source license for the project. The CI workflow badge
already correctly references the existing .github/workflows/ci.yml file, so no
changes are needed for that badge.
2 participants
Closes #391
Problem
sendToWebhookuseshttp.Postwhich relies onhttp.DefaultClientwith no timeout. A stalled webhook endpoint can block the scheduled job goroutine indefinitely.Fix
Use an
http.Clientwith a 30-second timeout for webhook delivery so stalled receivers fail fast with a clear error instead of hanging.Summary by CodeRabbit
New Features
Documentation
Bug Fixes