From 7b64f25d0e0f7f8a68fc97009d694b147dd380ac Mon Sep 17 00:00:00 2001 From: piekstra Date: Sun, 19 Jul 2026 15:49:06 -0400 Subject: [PATCH 1/2] fix(github): bound the default HTTP client with a timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub client fell back to http.DefaultClient, which has no timeout: a connection the server's edge silently abandons parks the request — and with it the whole review run — indefinitely. Observed in the wild as completed runs lingering ~27 minutes holding nothing but an ESTABLISHED github socket, until an external supervisor killed them. All requests on this client are REST/GraphQL JSON exchanges, so a 2-minute total-request timeout is generous. Callers that supply their own HTTPClient keep full control. --- internal/gitprovider/github/app_auth.go | 2 +- internal/gitprovider/github/client.go | 7 ++++++- internal/gitprovider/github/client_test.go | 12 ++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/internal/gitprovider/github/app_auth.go b/internal/gitprovider/github/app_auth.go index c2bd1b9..abec741 100644 --- a/internal/gitprovider/github/app_auth.go +++ b/internal/gitprovider/github/app_auth.go @@ -70,7 +70,7 @@ func newGitHubAppFromConfig(ctx context.Context, profile string, store credentia } httpClient := opts.HTTPClient if httpClient == nil { - httpClient = http.DefaultClient + httpClient = &http.Client{Timeout: defaultHTTPTimeout} } now := opts.Now if now == nil { diff --git a/internal/gitprovider/github/client.go b/internal/gitprovider/github/client.go index 586fe6b..eed012a 100644 --- a/internal/gitprovider/github/client.go +++ b/internal/gitprovider/github/client.go @@ -20,6 +20,11 @@ const ( credentialTypePAT = "pat" credentialTypeGitHubApp = "github_app" // #nosec G101 -- credential type label, not a secret. gitHubAppInitTimeout = 30 * time.Second + // defaultHTTPTimeout bounds every REST/GraphQL request made with the + // default client. All responses here are JSON payloads, so two minutes is + // generous — the bound exists because an unbounded client parks the whole + // run indefinitely on a connection the server's edge silently abandoned. + defaultHTTPTimeout = 2 * time.Minute gitHubAppRefreshSkew = 5 * time.Minute gitHubAppJWTBackdate = 60 * time.Second gitHubAppJWTLifetime = 9 * time.Minute @@ -135,7 +140,7 @@ func New(opts Options) (*Client, error) { } httpClient := opts.HTTPClient if httpClient == nil { - httpClient = http.DefaultClient + httpClient = &http.Client{Timeout: defaultHTTPTimeout} } return &Client{ host: normalizedHost, diff --git a/internal/gitprovider/github/client_test.go b/internal/gitprovider/github/client_test.go index 44e2b25..4e2aab5 100644 --- a/internal/gitprovider/github/client_test.go +++ b/internal/gitprovider/github/client_test.go @@ -31,6 +31,18 @@ func TestCapabilities(t *testing.T) { } } +func TestDefaultHTTPClientIsBounded(t *testing.T) { + // A stalled connection must never park a run indefinitely: the fallback + // client (used whenever the caller doesn't supply one) needs a timeout. + client := mustClient(t, Options{Token: "token"}) + if client.httpClient.Timeout <= 0 { + t.Fatalf("default HTTP client has no timeout; a silently dropped connection would hang forever") + } + if client.httpClient == http.DefaultClient { + t.Fatalf("default HTTP client must not be http.DefaultClient (shared, unbounded)") + } +} + func TestNewFromGitConfigBuildsPATClientAndCredential(t *testing.T) { store := tokenStore{"work": {credentials.GitTokenKey: "token"}} client, credential, err := NewFromGitConfig(config.GitConfig{ From 7dc91412eebf6c9fe08f8e49f9661904c2951a7c Mon Sep 17 00:00:00 2001 From: piekstra Date: Sun, 19 Jul 2026 16:01:35 -0400 Subject: [PATCH 2/2] refactor(github): share the bounded default client across construction paths Both the PAT and GitHub-App constructors now use one defaultBoundedHTTPClient helper, and the test covers the shared fallback directly. --- internal/gitprovider/github/app_auth.go | 2 +- internal/gitprovider/github/client.go | 10 +++++++++- internal/gitprovider/github/client_test.go | 13 +++++++++---- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/internal/gitprovider/github/app_auth.go b/internal/gitprovider/github/app_auth.go index abec741..d29056d 100644 --- a/internal/gitprovider/github/app_auth.go +++ b/internal/gitprovider/github/app_auth.go @@ -70,7 +70,7 @@ func newGitHubAppFromConfig(ctx context.Context, profile string, store credentia } httpClient := opts.HTTPClient if httpClient == nil { - httpClient = &http.Client{Timeout: defaultHTTPTimeout} + httpClient = defaultBoundedHTTPClient() } now := opts.Now if now == nil { diff --git a/internal/gitprovider/github/client.go b/internal/gitprovider/github/client.go index eed012a..06a9be6 100644 --- a/internal/gitprovider/github/client.go +++ b/internal/gitprovider/github/client.go @@ -140,7 +140,7 @@ func New(opts Options) (*Client, error) { } httpClient := opts.HTTPClient if httpClient == nil { - httpClient = &http.Client{Timeout: defaultHTTPTimeout} + httpClient = defaultBoundedHTTPClient() } return &Client{ host: normalizedHost, @@ -151,6 +151,14 @@ func New(opts Options) (*Client, error) { }, nil } +// defaultBoundedHTTPClient is the fallback for construction paths where the +// caller supplies no HTTPClient. Never http.DefaultClient: it has no timeout, +// so a connection the server's edge silently abandons would park the request +// — and the whole run — indefinitely. +func defaultBoundedHTTPClient() *http.Client { + return &http.Client{Timeout: defaultHTTPTimeout} +} + // Host returns the normalized host this client is bound to. func (c *Client) Host() string { if c == nil { diff --git a/internal/gitprovider/github/client_test.go b/internal/gitprovider/github/client_test.go index 4e2aab5..5bf29f1 100644 --- a/internal/gitprovider/github/client_test.go +++ b/internal/gitprovider/github/client_test.go @@ -33,14 +33,19 @@ func TestCapabilities(t *testing.T) { func TestDefaultHTTPClientIsBounded(t *testing.T) { // A stalled connection must never park a run indefinitely: the fallback - // client (used whenever the caller doesn't supply one) needs a timeout. - client := mustClient(t, Options{Token: "token"}) - if client.httpClient.Timeout <= 0 { + // client (used by both the PAT and GitHub-App construction paths whenever + // the caller doesn't supply one) needs a timeout. + fallback := defaultBoundedHTTPClient() + if fallback.Timeout <= 0 { t.Fatalf("default HTTP client has no timeout; a silently dropped connection would hang forever") } - if client.httpClient == http.DefaultClient { + if fallback == http.DefaultClient { t.Fatalf("default HTTP client must not be http.DefaultClient (shared, unbounded)") } + client := mustClient(t, Options{Token: "token"}) + if client.httpClient.Timeout <= 0 { + t.Fatalf("NewClient without an explicit HTTPClient must use the bounded fallback") + } } func TestNewFromGitConfigBuildsPATClientAndCredential(t *testing.T) {