Skip to content

fix: resolve context leak, unseeded jitter, and broken test utility #14

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.

Sign up for GitHub

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

Open
lupppig wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: resolve context leak, unseeded jitter, and broken test utility #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec84e53
fix: resolve context leak, unseeded jitter, and broken test utility
lupppig Jan 21, 2026
c1a1ee4
Change remote organization name in CLA workflow
lupppig Jan 21, 2026
b237cc7
Update remote organization name in CLA workflow
lupppig Jan 21, 2026
893137d
Update signed commit message in CLA workflow
lupppig Jan 25, 2026
eed53de
Modify signed commit message in CLA workflow
lupppig Jan 25, 2026
32c9a4d
fix: resolve context and connection leaks in request handler and test…
lupppig Jan 25, 2026
3597209
fix: resolve context leak and revert workflow changes
lupppig Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions internal/requestconfig/requestconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,8 @@ func retryDelay(res *http.Response, retryCount int) time.Duration {
delay = maxDelay
}

jitter := rand.Int63n(int64(delay / 4))
r := rand.New(rand.NewSource(time.Now().UnixNano()))
jitter := r.Int63n(int64(delay / 4))
delay -= time.Duration(jitter)
return delay
}
Expand Down Expand Up @@ -466,12 +467,6 @@ func (cfg *RequestConfig) Execute() (err error) {
ctx := cfg.Request.Context()
if cfg.RequestTimeout != time.Duration(0) && isBeforeContextDeadline(time.Now().Add(cfg.RequestTimeout), ctx) {
ctx, cancel = context.WithTimeout(ctx, cfg.RequestTimeout)
defer func() {
// The cancel function is nil if it was handed off to be handled in a different scope.
if cancel != nil {
cancel()
}
}()
}

req := cfg.Request.Clone(ctx)
Expand All @@ -480,17 +475,25 @@ func (cfg *RequestConfig) Execute() (err error) {
}

res, err = handler(req)

if ctx != nil && ctx.Err() != nil {
if cancel != nil {
cancel()
cancel = nil
}
Comment on lines 479 to +483
Copy link

@coderabbitai coderabbitai bot Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n internal/requestconfig/requestconfig.go | sed -n '470,495p'

Repository: logchimp/logchimp-go

Length of output: 773

🏁 Script executed:

cat -n internal/requestconfig/requestconfig.go | sed -n '450,520p'

Repository: logchimp/logchimp-go

Length of output: 2469

🏁 Script executed:

cat -n internal/requestconfig/requestconfig.go | sed -n '520,560p'

Repository: logchimp/logchimp-go

Length of output: 1371

🏁 Script executed:

cat -n internal/requestconfig/requestconfig.go | sed -n '400,450p'

Repository: logchimp/logchimp-go

Length of output: 1931

🌐 Web query:

http.Client.Do returns response with error Go standard library behavior

💡 Result:

In Go’s standard library (net/http), (*http.Client).Do(req) follows this contract:

  • HTTP status codes (including 4xx/5xx) do not produce an error. You get resp != nil, err == nil and must read/close resp.Body. [1]
  • err != nil means either:
    • a client policy error (most commonly redirect handling via CheckRedirect), or
    • a failure to speak HTTP (network/protocol/TLS, etc.). [1]
  • The only standard-library case where both resp != nil and err != nil occur is when CheckRedirect fails (including the default redirect policy such as “stopped after too many redirects”). In that case, resp.Body is already closed. [1][2]
  • Any error returned by Do is a *url.Error (useful for Timeout() checks, etc.). [1]

Practical handling pattern:

resp, err := client.Do(req)
if err != nil {
    // resp may be non-nil only on CheckRedirect failure; Body is already closed then.
    return err
}
defer resp.Body.Close()

Sources: [1] net/http docs for Client.Do (pkg.go.dev) (pkg.go.dev), [2] source-level explanation/examples of the redirect special-case (stackoverflow.com)

Citations:

🏁 Script executed:

rg -n "CustomHTTPDoer" internal/requestconfig/requestconfig.go

Repository: logchimp/logchimp-go

Length of output: 166

🏁 Script executed:

rg -B 5 -A 5 "interface.*HTTPDoer\|type HTTPDoer" internal/

Repository: logchimp/logchimp-go

Length of output: 46

🏁 Script executed:

rg -n "HTTPDoer" --type go

Repository: logchimp/logchimp-go

Length of output: 465

🏁 Script executed:

cat -n internal/requestconfig/requestconfig.go | sed -n '230,240p'

Repository: logchimp/logchimp-go

Length of output: 561

🏁 Script executed:

cat -n internal/requestconfig/requestconfig.go | sed -n '1,30p'

Repository: logchimp/logchimp-go

Length of output: 889

Close response bodies in all error paths to avoid connection leaks with custom HTTPDoer implementations.

While http.Client.Do typically doesn't return non-nil response with context errors, this code supports custom HTTPDoer implementations that may violate this contract. Additionally, the code inconsistently closes response bodies: it properly closes in the retry loop (line 506-508) but not in error paths.

Two locations need fixing:

  • Line 479-484: ctx.Err() return path
  • Line 529-534: handler error return path
🐛 Suggested fix 
 if ctx != nil && ctx.Err() != nil {
+	if res != nil && res.Body != nil {
+		res.Body.Close()
+	}
 	if cancel != nil {
 		cancel()
 		cancel = nil
 	}
 	return ctx.Err()
 }
 ...
 if err != nil {
+	if res != nil && res.Body != nil {
+		res.Body.Close()
+	}
 	if cancel != nil {
 		cancel()
 		cancel = nil
 	}
 	return 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.

Suggested change
if ctx != nil && ctx.Err() != nil {
if cancel != nil {
cancel()
cancel = nil
}
if ctx != nil && ctx.Err() != nil {
if res != nil && res.Body != nil {
res.Body.Close()
}
if cancel != nil {
cancel()
cancel = nil
}
🤖 Prompt for AI Agents 
In `@internal/requestconfig/requestconfig.go` around lines 479 - 483, In the
ctx.Err() and handler error-return paths, ensure any non-nil *http.Response
returned by the custom HTTPDoer is closed to avoid connection leaks: before
calling cancel() or returning on error, check if resp != nil and call
resp.Body.Close() (ignoring the close error) and then set resp = nil; apply this
change in the ctx.Err() branch (where cancel is cleared) and in the handler
error-return branch (where handler returns err) so both error paths mirror the
retry-loop cleanup that currently closes resp.Body.

return ctx.Err()
}
if !shouldRetry(cfg.Request, res) || retryCount >= cfg.MaxRetries {
break
}

// Prepare next request and wait for the retry delay
if cfg.Request.GetBody != nil {
cfg.Request.Body, err = cfg.Request.GetBody()
if err != nil {
if cancel != nil {
cancel()
cancel = nil
}
return err
}
}
Expand All @@ -500,10 +503,13 @@ func (cfg *RequestConfig) Execute() (err error) {
break
}

// Close the response body before retrying to prevent connection leaks
if res != nil && res.Body != nil {
res.Body.Close()
}
if cancel != nil {
cancel()
cancel = nil
}

time.Sleep(retryDelay(res, retryCount))
}
Expand All @@ -521,12 +527,20 @@ func (cfg *RequestConfig) Execute() (err error) {
// If there was a connection error in the final request or any other transport error,
// return that early without trying to coerce into an APIError.
if err != nil {
if cancel != nil {
cancel()
cancel = nil
}
return err
}

if res.StatusCode >= 400 {
contents, err := io.ReadAll(res.Body)
res.Body.Close()
if cancel != nil {
cancel()
cancel = nil
}
if err != nil {
return err
}
Expand Down Expand Up @@ -558,6 +572,10 @@ func (cfg *RequestConfig) Execute() (err error) {

contents, err := io.ReadAll(res.Body)
res.Body.Close()
if cancel != nil {
cancel()
cancel = nil
}
if err != nil {
return fmt.Errorf("error reading response body: %w", err)
}
Expand Down
10 changes: 7 additions & 3 deletions internal/testutil/testutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import (
)

func CheckTestServer(t *testing.T, url string) bool {
if _, err := http.Get(url); err != nil {
res, err := http.Get(url)
if err == nil {
res.Body.Close()
}
if err != nil {
const SKIP_MOCK_TESTS = "SKIP_MOCK_TESTS"
if str, ok := os.LookupEnv(SKIP_MOCK_TESTS); ok {
skip, err := strconv.ParseBool(str)
Expand All @@ -19,9 +23,9 @@ func CheckTestServer(t *testing.T, url string) bool {
t.Skip("The test will not run without a mock Prism server running against your OpenAPI spec")
return false
}
t.Errorf("The test will not run without a mock Prism server running against your OpenAPI spec. You can set the environment variable %s to true to skip running any tests that require the mock server", SKIP_MOCK_TESTS)
return false
}
t.Errorf("The test will not run without a mock Prism server running against your OpenAPI spec. You can set the environment variable %s to true to skip running any tests that require the mock server. Error: %s", SKIP_MOCK_TESTS, err)
return false
}
return true
}